Skip to content

Lint fails with error #429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bavulapati opened this issue May 11, 2025 · 11 comments
Open

Lint fails with error #429

bavulapati opened this issue May 11, 2025 · 11 comments

Comments

@bavulapati
Copy link
Contributor

Linting is not run on CI

Lint errors
      ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:206:14: error: enum 'EntryType' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size,-warnings-as-errors]
  206 |   enum class EntryType { Push, Pass, Fail, Annotation };
      |              ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:209:21: error: member 'type' of type 'const EntryType' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  209 |     const EntryType type;
      |                     ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:210:28: error: member 'name' of type 'const std::string_view' (aka 'const basic_string_view<char>') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  210 |     const std::string_view name;
      |                            ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:211:41: error: member 'instance_location' of type 'const sourcemeta::core::WeakPointer' (aka 'const GenericPointer<reference_wrapper<const basic_string<char>>, PropertyHashJSON<basic_string<char, char_traits<char>, allocator<char>>>>') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  211 |     const sourcemeta::core::WeakPointer instance_location;
      |                                         ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:212:41: error: member 'evaluate_path' of type 'const sourcemeta::core::WeakPointer' (aka 'const GenericPointer<reference_wrapper<const basic_string<char>>, PropertyHashJSON<basic_string<char, char_traits<char>, allocator<char>>>>') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  212 |     const sourcemeta::core::WeakPointer evaluate_path;
      |                                         ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:213:23: error: member 'keyword_location' of type 'const std::string' (aka 'const basic_string<char>') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  213 |     const std::string keyword_location;
      |                       ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:214:49: error: member 'annotation' of type 'const std::optional<sourcemeta::core::JSON>' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  214 |     const std::optional<sourcemeta::core::JSON> annotation;
      |                                                 ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:217:16: error: no header providing "std::pair" is directly included [misc-include-cleaner,-warnings-as-errors]
    4 |     const std::pair<bool, std::optional<std::string>> vocabulary;
      |                ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:217:55: error: member 'vocabulary' of type 'const std::pair<bool, std::optional<std::string>>' (aka 'const pair<bool, optional<basic_string<char>>>') is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members,-warnings-as-errors]
  217 |     const std::pair<bool, std::optional<std::string>> vocabulary;
      |                                                       ^
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:228:3: error: function 'begin' should be marked [[nodiscard]] [modernize-use-nodiscard,-warnings-as-errors]
  228 |   auto begin() const -> const_iterator;
      |   ^
      |   [[nodiscard]] 
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:229:3: error: function 'end' should be marked [[nodiscard]] [modernize-use-nodiscard,-warnings-as-errors]
  229 |   auto end() const -> const_iterator;
      |   ^
      |   [[nodiscard]] 
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:230:3: error: function 'cbegin' should be marked [[nodiscard]] [modernize-use-nodiscard,-warnings-as-errors]
  230 |   auto cbegin() const -> const_iterator;
      |   ^
      |   [[nodiscard]] 
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_output.h:231:3: error: function 'cend' should be marked [[nodiscard]] [modernize-use-nodiscard,-warnings-as-errors]
  231 |   auto cend() const -> const_iterator;
      |   ^
      |   [[nodiscard]] 
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_unevaluated.h:8:1: error: included header json.h is not used directly [misc-include-cleaner,-warnings-as-errors]
    8 | #include <sourcemeta/core/json.h>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    9 | #include <sourcemeta/core/jsonpointer.h>
/home/personal/repos/sourcemeta/blaze/src/compiler/include/sourcemeta/blaze/compiler_unevaluated.h:10:1: error: included header jsonschema.h is not used directly [misc-include-cleaner,-warnings-as-errors]
   10 | #include <sourcemeta/core/jsonschema.h>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   11 | 
@jviotti
Copy link
Member

jviotti commented May 12, 2025

In general I was avoiding ClangTidy on CI because it's pretty slow. How long did it take on your machine?

We can always try to add a new job for it, and also feel free to send small PRs fixing all of the little things its complaining about in the mean-time.

@bavulapati
Copy link
Contributor Author

It takes 4 minutes on my intel 11th gen i7 8 core CPU. My fan makes a lot of noise too during this run.

Can we run clang-tidy on one file at a time?
I currently run

make lint

and the output is too long.

@bavulapati
Copy link
Contributor Author

Edited the sources on CMakeLists.txt

bavulapati added a commit to bavulapati/blaze that referenced this issue May 12, 2025
bavulapati added a commit to bavulapati/blaze that referenced this issue May 12, 2025
bavulapati added a commit to bavulapati/blaze that referenced this issue May 12, 2025
    Refs sourcemeta#429

Signed-off-by: Balakrishna Avulapati <[email protected]>

fix clang_format issue

Signed-off-by: Balakrishna Avulapati <[email protected]>
@jviotti
Copy link
Member

jviotti commented May 12, 2025

It takes 4 minutes on my intel 11th gen i7 8 core CPU. My fan makes a lot of noise too during this run.

Let me try on GitHub Actions. Blaze already spends a lot of time on tests and benchmarks, so maybe it's good enough.

Can we run clang-tidy on one file at a time?

I don't think so, but I might be wrong. In any case, we probably want to run it in the entire project?


Also, as a final note, maybe we should make ClangTidy pass on https://github.com/sourcemeta/core first, as I wonder if the linting results from that repo will cascade here.

@bavulapati
Copy link
Contributor Author

Also, as a final note, maybe we should make ClangTidy pass on https://github.com/sourcemeta/core first, as I wonder if the linting results from that repo will cascade here.

Will see if core has linting errors

jviotti added a commit that referenced this issue May 12, 2025
See: #429
Signed-off-by: Juan Cruz Viotti <[email protected]>
jviotti added a commit that referenced this issue May 12, 2025
See: #429
Signed-off-by: Juan Cruz Viotti <[email protected]>
jviotti added a commit to sourcemeta/core that referenced this issue May 12, 2025
@jviotti
Copy link
Member

jviotti commented May 12, 2025

Here are some PRs running ClangTidy on CI:

Feel free to send PRs fixing the warnings and we can rebase when we think it's all green and see how long it takes

@jviotti
Copy link
Member

jviotti commented May 12, 2025

I believe most of the warnings can be easily fixed. Some might require discussion. And there are probably some outliers that we don't want to fix. For example, I remember ClangTidy complaining about the use of goto in the JSON parser, but we do want to keep those goto for performance reasons (even if most linters discourage them!). In those cases, there is a syntax to disable ClangTidy rules for specific lines using comments (like other linters)

@jviotti
Copy link
Member

jviotti commented May 12, 2025

~6 minutes for Blaze: https://github.com/sourcemeta/blaze/actions/runs/14978789772/job/42077663860?pr=433. Not bad. Windows builds take a lot longer already 😅

Image

jviotti added a commit that referenced this issue May 12, 2025
See: #429
Signed-off-by: Juan Cruz Viotti <[email protected]>
@bavulapati
Copy link
Contributor Author

Clang_tidy might run longer on core

@jviotti
Copy link
Member

jviotti commented May 12, 2025

It was not that bad: ~7m. https://github.com/sourcemeta/core/actions/runs/14978825522/job/42077781489?pr=1640. I think it's acceptable now that the project is a lot bigger. Once more, MSVC builds on Core take even longer haha

Image

@jviotti
Copy link
Member

jviotti commented May 12, 2025

Lots of very minor things to fix. I guess its a good exercise on C++ knowledge

jviotti pushed a commit that referenced this issue May 12, 2025
bavulapati added a commit to bavulapati/core that referenced this issue May 13, 2025
json_error.h is indirectly included via json.h
Refs sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
jviotti pushed a commit to sourcemeta/core that referenced this issue May 13, 2025
json_error.h is indirectly included via json.h
Refs sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
bavulapati added a commit to bavulapati/core that referenced this issue May 18, 2025
Reported by clang-tidy check `misc-include-cleaner`
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
bavulapati added a commit to bavulapati/core that referenced this issue May 18, 2025
    Reported by clang-tidy check `misc-include-cleaner`
    Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
bavulapati added a commit to bavulapati/core that referenced this issue May 18, 2025
Reported by clang-tidy check `misc-include-cleaner`
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
bavulapati added a commit to bavulapati/core that referenced this issue May 18, 2025
Reported by clang-tidy check misc-include-cleaner
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
jviotti pushed a commit to sourcemeta/core that referenced this issue May 21, 2025
Reported by clang-tidy check `misc-include-cleaner` 
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
jviotti pushed a commit to sourcemeta/core that referenced this issue May 21, 2025
Reported by clang-tidy check misc-include-cleaner
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
jviotti pushed a commit to sourcemeta/core that referenced this issue May 21, 2025
Reported by clang-tidy check misc-include-cleaner
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
bavulapati added a commit to bavulapati/core that referenced this issue May 21, 2025
Reported by clang-tidy check misc-include-cleaner
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
bavulapati added a commit to bavulapati/core that referenced this issue May 23, 2025
Reported by clang-tidy check misc-include-cleaner
Refs: sourcemeta/blaze#429

Signed-off-by: Balakrishna Avulapati <[email protected]>
jviotti added a commit to sourcemeta/core that referenced this issue May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants