carlosgalvezp added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:23-40
+namespace {
+class VaArgPPCallbacks : public PPCallbacks {
+public:
+ VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {}
+
+ void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+ SourceRange Range, const MacroArgs *Args) override {
----------------
salman-javed-nz wrote:
> Should this be in another patch?
>
> Line 722 in ClangTidyDiagnosticConsumer.cpp makes it so that clang-tidy
> filters warnings from system macros. This would benefit all checks.
>
> This VaArgPPCallBack change here only benefits
> `cppcoreguidelines-pro-type-vararg`.
The change to `ClangTidyDiagnosticConsumer.cpp` broke this check, because it
was checking for the use of `vararg` indirectly, checking for the use of
`__builtin_vararg`, which was expanded from the system macro `vararg`. This
patch updates the code so it checks directly what the rule is about "do not use
`vararg`". I should perhaps have been more clear about that in the commit
message :)
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:46-78
+ "__builtin_isgreater",
+ "__builtin_isgreaterequal",
"__builtin_isless",
- "__builtin_islessequal",
- "__builtin_islessgreater",
+ "__builtin_islessequal",
+ "__builtin_islessgreater",
"__builtin_isunordered",
+ "__builtin_fpclassify",
----------------
salman-javed-nz wrote:
> Formatting changed for code not directly involved in this patch. Should be
> moved to a separate NFC commit that you don't have to run by us.
Yes, it's a bit unfortunate, this was an automatic change on save from my
editor. Is it really worth it to revert this change though? I will need to
disable this feature on my IDE, revert each line manually by adding a space,
push, then re-enable the feature. Sounds like a lot of time spent on reverting
a change that anyway improves the state of the code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116378/new/
https://reviews.llvm.org/D116378
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
