salman-javed-nz added a comment. In D111208#3053061 <https://reviews.llvm.org/D111208#3053061>, @carlosgalvezp wrote:
> Looking at the code, I see that you use `SuppressionIsSpecific` in order to > separate the errors into 2 error lists: `SpecificNolintBegins` and > `GlobalNolintBegins`. However you don't do anything different with either > list, so why do they need to be different lists? > > Here checking that a combined list is empty would be equivalent: > > bool WithinNolintBegin = > !SpecificNolintBegins.empty() || !GlobalNolintBegins.empty(); > > And here, you are running identical code for both lists: > > for (const auto NolintBegin : SpecificNolintBegins) { > auto Error = createNolintError(Context, SM, NolintBegin, true); > SuppressionErrors.emplace_back(Error); > } > for (const auto NolintBegin : GlobalNolintBegins) { > auto Error = createNolintError(Context, SM, NolintBegin, true); > SuppressionErrors.emplace_back(Error); > } > > And then these lists are not used any further than the scope of the function > where they are declared. So to me it feels like they could be combined, and > this logic of `SuppressionIsSpecific` be removed. Let me know if I'm missing > something obvious! `tallyNolintBegins()` goes through every line, and whenever it sees a `NOLINTBEGIN`, it pushes its SourceLocation onto a stack. When it sees a `NOLINTEND`, it pops one entry off the stack. At the end of the file, all entries on the stack should have been popped off - if not, it's a unmatched `NOLINTBEGIN/END` expression error. `NOLINTBEGIN(check-name)` expressions are pushed onto the `SpecificNolintBegins` list, while `NOLINTBEGIN` & `NOLINTBEGIN(*)` expressions are pushed onto the `GlobalNolintBegins` list. The reason for the two lists is so that `NOLINTBEGIN(check-name)` cannot be popped off by `NOLINTEND` or `NOLINTEND(*)`; and `NOLINTBEGIN` and `NOLINTBEGIN(*)` cannot be popped off by a `NOLINTEND(check-name)`. See this line in `tallyNolintBegins()` where it figures out which list to use for the `NOLINTBEGIN/END` expression currently being evaluated... auto List = [&]() -> SmallVector<SourceLocation> * { return SuppressionIsSpecific ? &SpecificNolintBegins : &GlobalNolintBegins; }; I'm sure there is an alternate implementation which achieves the same thing with just one list object... however the list will probably need to store more information about the `NOLINTBEGIN` expression than just its SourceLocation to be able to achieve the same goals though... probably a list of `pair<SourceLocation Location, StringRef CheckName>`, otherwise you can't match a `NOLINTEND` to its corresponding `NOLINTBEGIN`. If you want to take a crack at simplifying the logic here, all improvements are appreciated. ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + if (SuppressionIsSpecific) + *SuppressionIsSpecific = true; } ---------------- carlosgalvezp wrote: > salman-javed-nz wrote: > > salman-javed-nz wrote: > > > carlosgalvezp wrote: > > > > salman-javed-nz wrote: > > > > > So when this function is called with args `Line = > > > > > "NOLINTBEGIN(google*)"` and `CheckName = > > > > > "google-explicit-constructor"`, it will return true with > > > > > `*SuppressionIsSpecific = true`. > > > > > Should `*SuppressionIsSpecific = false` instead here? > > > > Good point. I honestly don't understand what SuppressionIsSpecific > > > > means and how it's used downstream, could you clarify? > > > `SuppressionIsSpecific = true` means when a check is suppressed by name, > > > i.e. `NOLINTBEGIN(check-name)`. > > > > > > `SuppressionIsSpecific = false` means when `NOLINTBEGIN` <zero args> or > > > `NOLINTBEGIN(*)` is used. My gut feeling is that It should probably be > > > false when a glob is used too. > > > > > > The point of this variable is so that a check-specific `BEGIN` can only > > > be `END`ed with a check-specific END. > > I think the behaviour has changed in this patch. > > > > It used to return `*SuppressionIsSpecific = false` when `ChecksStr = "*"`. > > `"*"` is disabling all checks, not a specific check. > > > > Now it returns `*SuppressionIsSpecific = true`. `Globs.contains(CheckName)` > > is true regardless of whether `ChecksStr = "*"` or `ChecksStr = > > "check-name"`, so it continues running onto line 353 and sets > > `*SuppressionIsSpecific = true` no matter what. > You are right! It's interesting however that this hasn't triggered any unit > test error, so I wonder if this variable is needed at all then? I will have a > deeper look at the code to get a better understanding and update the patch > accordingly. > You are right! It's interesting however that this hasn't triggered any unit > test error That's because the unit tests are missing a test case that checks this specific combination of BEGIN/END expressions. The tests predominantly use `NOLINTBEGIN` <zero args>, not `NOLINTBEGIN(*)`. It would be great if this shortcoming in the test coverage could be plugged in this patch. `nolintbeginend-begin-global-end-specific.cpp` checks: ``` // NOLINTBEGIN class A { A(int i); }; // NOLINTEND(google-explicit-constructor) ``` `nolintbeginend-begin-specific-end-global.cpp` checks: ``` // NOLINTBEGIN(google-explicit-constructor) class A { A(int i); }; // NOLINTEND ``` These 2 files could be used as the basis to create tests for `NOLINTBEGIN(*)` / `NOLINTEND(*)`. ================ Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347 + // No warnings are suppressed, due to double negation + Foo(bool param); // NOLINT(-google*) }; ---------------- carlosgalvezp wrote: > salman-javed-nz wrote: > > salman-javed-nz wrote: > > > carlosgalvezp wrote: > > > > salman-javed-nz wrote: > > > > > Would anyone do this on purpose, or is this a user error? > > > > I don't see any use case here, no. I just thought to document it to > > > > clarify the double negation that's happening, in case a user gets > > > > confused and doesn't see the warning being suppressed. > > > > > > > > Do you think it brings value or does more harm than good? > > > > > > I don't see any use case here, no. I just thought to document it to > > > clarify the double negation that's happening, in case a user gets > > > confused and doesn't see the warning being suppressed. > > > > > > Do you think it brings value or does more harm than good? > > > > I'd be happy with just the basic `+` glob functionality. The first thing > > I'd use it on is to suppress checks that are an alias of another check, > > e.g. `NOLINT(*no-malloc)` to cover both `hicpp-no-malloc` and > > `cppcoreguidelines-no-malloc`. > > > > As for glob expressions that begin with a `-`... you get the functionality > > for free thanks to the `GlobList` class but it's not a feature I think I > > will need. I speak only for myself though. Maybe someone else here has a > > strong need for this? > > > > Is `NOLINTBEGIN(-check)` equivalent to `NOLINTEND(check)`? It hursts my > > head even thinking about it. > > > > Your unit test where you test `NOLINT(google*,-google*,google*)`, > > Clang-Tidy does the right thing in that situation, but I have to wonder why > > any user would want to add, remove, then add the same check group in the > > one expression in the first place? Should we even be entertaining this kind > > of use? > > Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)? > > To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching > NOLINTEND(X), regardless of whether X is a glob or not. > > Totally agree that negative globs are probably not needed, the main use case > is being able to suppress all aliases as you say. > > I just thought it was neat to be able to reuse code in that way, the code is > very clean and easy to read. Plus users are familiar with the syntax. If > users want to abuse it that would be at their own maintenance expense - > clang-tidy would just do what it was asked to do, without crashing or > anything bad happening. > > The same thing can be done when running the tool - you can run > "--checks=google*,-google*" and I don't think clang-tidy has code to warn the > user/prevent this from happening. Considering all these edge cases would > perhaps complicate the design of the tool for no real use case? I.e. too > defensive programming. > > I added the unit test mostly to make sure clang-tidy doesn't crash or does > something strange, it just does what the user instructed it to do. I thought > such edge cases are encouraged in unit tests to increase coverage. > > Would it be reasonable to keep negative globs in the implementation (to > simplify maintenance, reuse code), but not document it in the public docs? > Otherwise I'll need to refactor GlobList to be able to reuse the part of the > code that consumes the positive globs. I don't think it makes sense to create > a parallel implementation of that part (kind of what exists already now, > before my patch). > > Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)? > > To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching > NOLINTEND(X), regardless of whether X is a glob or not. > > Totally agree that negative globs are probably not needed, the main use case > is being able to suppress all aliases as you say. > > I just thought it was neat to be able to reuse code in that way, the code is > very clean and easy to read. Plus users are familiar with the syntax. If > users want to abuse it that would be at their own maintenance expense - > clang-tidy would just do what it was asked to do, without crashing or > anything bad happening. > > The same thing can be done when running the tool - you can run > "--checks=google*,-google*" and I don't think clang-tidy has code to warn the > user/prevent this from happening. Considering all these edge cases would > perhaps complicate the design of the tool for no real use case? I.e. too > defensive programming. > > I added the unit test mostly to make sure clang-tidy doesn't crash or does > something strange, it just does what the user instructed it to do. I thought > such edge cases are encouraged in unit tests to increase coverage. > I see all your points about reusing the glob functionality. As long as this functionality does not let a user shoot themselves in the foot, I don't see a problem with it. A large proportion of the review of my `NOLINTBEGIN/NOLINTEND` patch was spent going over all the ways a user could misuse the functionality. The concern was mainly that we didn't want a user to unwittingly disable checks for the entire file because of a stray `NOLINTBEGIN` without an `NOLINTEND` marker. The worst I can see a confused user do with this new glob feature is use `NOLINT(-check*)` which as you have already explained is a double negative and shows the warnings anyway, so no harm done. I find it funny that master allows `NOLINT(` with no closing bracket go though as a `NOLINT` when it's clearly a sign of user error. Clang-Tidy could be more proactive in alerting the user to these mistakes, but I get there's a balance between code complexity and harm reduction. This quirk is not directly related to your patch so that's a story to continue on another day. > Would it be reasonable to keep negative globs in the implementation (to > simplify maintenance, reuse code), but not document it in the public docs? > Otherwise I'll need to refactor GlobList to be able to reuse the part of the > code that consumes the positive globs. I don't think it makes sense to create > a parallel implementation of that part (kind of what exists already now, > before my patch). I'm only here to provide some insight about code I recently wrote. Let's wait for the actual reviewers who have a better sense of the direction of this project to say how things should be. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits