Page MenuHomePhabricator

salman-javed-nz (Salman Javed)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 23 2021, 6:51 AM (9 w, 5 h)

Recent Activity

Sat, Oct 23

salman-javed-nz added a comment to D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Definitely an improvement! I looked at all the changed places and found some more improvements you can make. I don't need to see this again, though.

Sat, Oct 23, 4:11 PM · Restricted Project
salman-javed-nz added a comment to D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Awesome @salman-javed-nz , thanks for fixing the docs! May I ask how did you create these "smaller commits"? It's very nice to click on each of them and see only what changed. I believe I followed your same procedure (use the "Update Revision" thingy) but it always displays the full commit with all changes, so it's hard to see what each new update did.

Sat, Oct 23, 4:34 AM · Restricted Project

Fri, Oct 22

salman-javed-nz added a comment to D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Thanks for the review.
Are you able to commit for me?

Fri, Oct 22, 11:44 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Pre-merge checks failing because patch cannot be applied.
Therefore recreating patch to fix this.

Fri, Oct 22, 9:21 PM · Restricted Project
salman-javed-nz added a comment to D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

A collection of minor corrections that I don't think deserve separate commits.

Fri, Oct 22, 8:35 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Fix spelling and grammatical mistakes found across the .rst files.

Fri, Oct 22, 8:34 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Standardize to US English spelling for words such as "behavior" and "specialization".

Fri, Oct 22, 8:33 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Remove repeated words, e.g. "for a a larger user input".

Fri, Oct 22, 8:31 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Standardize spelling of "e.g." and "i.e."

Fri, Oct 22, 8:30 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Replace curly quote characters (‘ ’ “ ”) with standard straight ones (' ").
Replace en-dash () with standard hyphen (-).
In both cases, the latter is used more often in the doc files and is easier to type.

Fri, Oct 22, 8:29 PM · Restricted Project
salman-javed-nz updated the diff for D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.

Change double spaces after commas and periods to single space.

Fri, Oct 22, 8:28 PM · Restricted Project
salman-javed-nz requested review of D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation.
Fri, Oct 22, 8:27 PM · Restricted Project

Wed, Oct 20

salman-javed-nz added inline comments to rGd8afa5777b66: [clang-tidy] Fix documentation typos (NFC).
Wed, Oct 20, 2:46 PM

Tue, Oct 12

salman-javed-nz added inline comments to D111208: [clang-tidy] Support globbing in NOLINT* expressions.
Tue, Oct 12, 4:09 AM · Restricted Project
salman-javed-nz added inline comments to D111208: [clang-tidy] Support globbing in NOLINT* expressions.
Tue, Oct 12, 4:03 AM · Restricted Project

Sun, Oct 10

salman-javed-nz added a comment to D111208: [clang-tidy] Support globbing in NOLINT* expressions.

Thanks for all the changes. You've addressed all my questions ad I don't have anything more to add. Let's wait for the reviewers to take a look.

Sun, Oct 10, 1:51 PM · Restricted Project
salman-javed-nz added inline comments to D111208: [clang-tidy] Support globbing in NOLINT* expressions.
Sun, Oct 10, 11:25 AM · Restricted Project
salman-javed-nz added inline comments to D111208: [clang-tidy] Support globbing in NOLINT* expressions.
Sun, Oct 10, 4:15 AM · Restricted Project
salman-javed-nz added a comment to D111208: [clang-tidy] Support globbing in NOLINT* expressions.

Now, the requirements for a match are stricter (and simpler), making the code easier to reason about: any NOLINTBEGIN(X) must be matched by an identical NOLINTEND(X), for any X.

Thanks! Simplifying it right down looks to me like the right thing to do.

Sun, Oct 10, 4:07 AM · Restricted Project

Sat, Oct 9

salman-javed-nz added a comment to D111208: [clang-tidy] Support globbing in NOLINT* expressions.

By the way, is NOLINTBEGIN/END expected to work/give errors when the check name is something else than a real check name? See for example:

https://godbolt.org/z/b6cbTeezs

I would expect to get a warning that an END was found that didn't match a BEGIN.

The functions that we have been discussing in this patch only run when Clang-Tidy has detected a check violation and is about to print a warning about it. The NOLINT comment checks happen at the last minute. Since foo and bar are not real checks, Clang-Tidy won't find any violations, and so the NOLINT checks won't get the opportunity to run.

Sat, Oct 9, 2:55 PM · Restricted Project
salman-javed-nz added a comment to D111208: [clang-tidy] Support globbing in NOLINT* expressions.

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!

Sat, Oct 9, 3:46 AM · Restricted Project
salman-javed-nz added inline comments to D111208: [clang-tidy] Support globbing in NOLINT* expressions.
Sat, Oct 9, 12:04 AM · Restricted Project

Fri, Oct 8

salman-javed-nz added a comment to D111208: [clang-tidy] Support globbing in NOLINT* expressions.

How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs with globbed ones?

I would say keep current behavior - complain that the user wrote something different in the BEGIN and in the END. I like keeping things simple and easy to read and reason about. I think adding smarter logic here would make clang-tidy's code more complicated, but also it would allow users to write more complicated NOLINT expressions that would be very hard to read and track.

Honestly as a user I'm more than fine with one level of NOLINTBEGIN/END. Any more nesting than that causes me much more cognitive effort than it's worth.

What do you think?

Fri, Oct 8, 5:15 AM · Restricted Project
salman-javed-nz added a comment to D111208: [clang-tidy] Support globbing in NOLINT* expressions.

What should happen with the following code:

// NOLINTBEGIN(google*)
Fri, Oct 8, 2:15 AM · Restricted Project

Thu, Oct 7

salman-javed-nz added inline comments to D111208: [clang-tidy] Support globbing in NOLINT* expressions.
Thu, Oct 7, 11:34 PM · Restricted Project

Tue, Sep 28

salman-javed-nz added a comment to rGc0687e1984a8: Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Hi,
One of the tests added in this change is failing on PowerPC buildbots. One such bot is https://lab.llvm.org/buildbot/#/builders/121/builds/11903. Please revert and let us know how we can help you figure out what needs to be done to investigate/fix this on the PPC bots.
Thanks!

Tue, Sep 28, 4:19 PM
salman-javed-nz added a comment to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.


You can see in the attached file, that when I ran the unit test on a x86_64 Windows 10 PC, the diagnostic from error_in_include.inc is printed at the end. However, on this this clang-s390x-linux-multistage build, it is printing the diagnostic from error_in_include.inc first:
https://lab.llvm.org/buildbot/#/builders/8/builds/1860/steps/5/logs/FAIL__Clang_Tools__nolintbeginend_cpp.

Tue, Sep 28, 2:13 PM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Resolving build error due to failed unit test.

Tue, Sep 28, 1:50 PM · Restricted Project
salman-javed-nz added a comment to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Yes, I will need your help to commit.

Tue, Sep 28, 4:24 AM · Restricted Project
salman-javed-nz added a comment to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Also, the pre-merge build failure is confirmed to be a problem with the build system and not the contents of my patch.
https://github.com/google/llvm-premerge-checks/issues/353

Tue, Sep 28, 3:39 AM · Restricted Project
salman-javed-nz added inline comments to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
Tue, Sep 28, 3:21 AM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Addressing review comments.

Tue, Sep 28, 3:18 AM · Restricted Project

Sep 24 2021

salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Pre-merge build error doesn't seem related to my change, but rebasing patch anyway just to be sure.

Sep 24 2021, 2:55 PM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

lineIsWithinNolintBegin():
Put NOLINTBEGIN comments into 2 buckets:

  1. Comments that apply to a specific check - NOLINTBEGIN(check)
  2. Comments that apply to all checks - NOLINTBEGIN(*) and NOLINTBEGIN with no args

If a check is begun with one type of comment, it must be ended with the same type.

Sep 24 2021, 4:25 AM · Restricted Project
salman-javed-nz updated the summary of D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
Sep 24 2021, 4:23 AM · Restricted Project
salman-javed-nz added a comment to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Sorry for not thinking of this sooner, but there is another diagnostic we might want to consider.

// NOLINTBEGIN(check)
// NOLINTEND(other-check)

where the file does not contain a NOLINTEND(check) comment anywhere. In this case, the markers are not actually matched, so it's a NOLINTBEGIN(check) comment with no end and a NOLINTEND(other-check) comment with no begin. That seems like user confusion we'd also want to diagnose, but it also could be tricky because you really want to add diagnostic arguments for the check name.

Sep 24 2021, 4:21 AM · Restricted Project

Sep 21 2021

salman-javed-nz added a comment to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Thanks, I think this is getting close! There are two more test cases that I think are interesting (and should cause any issues, hopefully):

// NOLINTEND
// CHECK-MESSAGES: for diagnostic on the previous line

and

// CHECK-MESSAGES: for diagnostic on the subsequent line
// NOLINTBEGIN

as the only contents in the file. The idea is that we want to check that searching for a "begin" when seeing an "end" at the top of the file doesn't do anything silly like try to read off the start of the file, and similar for "end" when seeing a "begin" at the end of a file.

Sep 21 2021, 8:46 AM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

lineIsWithinNolintBegin():

  • If the search through the preceding lines returns no active NOLINTBEGINs, carry on reading the rest of the file anyway.
Sep 21 2021, 8:34 AM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Updated according to review comments.

  • test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp: new test
  • test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp: new test
  • Use llvm::ErrorOr return type with getFile()
  • Split diagnostic message into 2 messages (one specific to NOLINTBEGIN and one specific to NOLINTEND)
  • if ... else brace wrapping
  • Spelling corrected to "nonexistent"
  • Assert that Unused diagnostic container is empty
  • change std::vector<ClangTidyError> to llvm::SmallVectorImpl
Sep 21 2021, 7:56 AM · Restricted Project

Sep 19 2021

salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Minor update to function signatures

  • Remove arguments that are not absolutely required
  • Added consts
Sep 19 2021, 7:57 PM · Restricted Project

Sep 18 2021

salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

shouldSuppressDiagnostic():

  • Changed to take a container of diagnostics as an argument. If it finds any stray NOLINTBEGIN/NOLINTEND markers, it creates a diagnostic regarding the stray marker and places it in the container.
Sep 18 2021, 8:13 AM · Restricted Project
salman-javed-nz added inline comments to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
Sep 18 2021, 7:55 AM · Restricted Project
salman-javed-nz updated the summary of D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
Sep 18 2021, 7:54 AM · Restricted Project

Sep 12 2021

salman-javed-nz added a comment to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Is this syntax used by any other tools?

It seems Google have implemented NOLINTBEGIN and NOLINTEND support in cpplint. I see lines such as // NOLINTBEGIN(whitespace/line_length), // NOLINTBEGIN(readability/check), // NOLINTBEGIN(build/include) scattered across different Google projects on GitHub.

Sep 12 2021, 8:37 PM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Changes in this latest patch:

  • LineIsMarkedWithNOLINT(): Moved NOLINTBEGIN/END aspects into a separate function.
  • LineIsWithinNOLINTBEGIN(): A NOLINTBEGIN/END region is only considered valid if both the BEGIN and END markers are present. If the region is malformed, the original warning will be raised.
Sep 12 2021, 8:05 PM · Restricted Project

Sep 10 2021

salman-javed-nz added inline comments to D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
Sep 10 2021, 12:54 AM · Restricted Project

Sep 8 2021

salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
  • Added test to check what happens when NOLINTEND marker is used before NOLINTBEGIN marker (class B1, line 7).
    • Warning is still displayed (NOLINTEND ends suppression but this is redundant as there was no suppression in the first place).
Sep 8 2021, 4:37 AM · Restricted Project

Sep 2 2021

salman-javed-nz updated subscribers of D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

@gonzalobg, @toklinke - I think you two might be interested in this because you have made proposals about this feature before.

Sep 2 2021, 2:08 PM · Restricted Project
salman-javed-nz updated the diff for D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.

Add additional checks in test/clang-tidy/infrastructure/nolintbeginend.cpp to check that error suppressions in one file are carried over when the file is #included in another file.

Sep 2 2021, 9:28 AM · Restricted Project

Aug 23 2021

salman-javed-nz requested review of D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments.
Aug 23 2021, 8:41 AM · Restricted Project