LG

In D49890#1200061, @TheAhmad wrote:

In D49890#1182556, @alexfh wrote:

Could you describe the specific problem you're solving and provide an example? As mentioned by others, a test would be very welcome as well.

Sorry for so much delay, @alexfh. I didn't see your comment. I will describe in detail:
I wanted to do a source to source transformation on MPlayer-1.3.0 source code. The transformation may require modification of many files and possibly repeated modifications in the headers files included in multiple .c files. Therefore, the changes should be serialized for each translation unit and stored in a YAML file. At the end, clang-apply-replacements will be called and transform the entire source code.
The problem is that clang-tidy expects a limited format for the compilation database. This is the format typically used when the build system generating the compilation database is CMAKE. But MPlayer uses Makefile. Therefore, I had to use an external database generator, Bear. In this case, the contents of the YAML files are OK. But it is not what is expected by clang-tidy. clang-tidy requires every file path to be absolute, even the header files.
The problem (i.e., using relative paths) only arises when the fixes are exported. Not when they are applied in-place. I reused some of the code for the in-place case and did some modifications to it. The code is OK, at least for my case with MPlayer. A small change is still needed to support merge conflicts which can be brought from the in-place fix stuff. It seems that at the end the commanlities of the two cases should be put in a function. Then this function can be called from both places (i.e., the in-place fix and the export fix).
I am new to Clang and do not know what is needed for tests. I am looking forward to your reply.
Regards.

In D54401#1320772, @Szelethus wrote:

In D54401#1318282, @alexfh wrote:

In D54401#1315354, @NoQ wrote:

The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via -checks=-analyzer-....

*summons @alexfh*

It's hard to tell without trying. Could you build and test clang-tools-extra with this patch? There should be a test for the relevant functionality in clang-tidy.

It compiles without problem, and check-all doesn't produce a single clang-tools-extra failure. Yay!

In D54945#1321301, @alexfh wrote:

In D54945#1318343, @MarinaKalashina wrote:

@alexfh Thanks a lot for your patience and help. I've made another revision, now with the diff made by 'git show HEAD -U999999' to have the full context availlable.

I don't know how this happened, but the diff is still almost empty.

In D54945#1318343, @MarinaKalashina wrote:

@alexfh Thanks a lot for your patience and help. I've made another revision, now with the diff made by 'git show HEAD -U999999' to have the full context availlable.

Could you send each check in a separate patch, please? This makes the review much more straightforward and I'm sure it will be faster too.

In D54945#1318283, @MarinaKalashina wrote:

In D54945#1318278, @alexfh wrote:

In D54945#1316251, @MarinaKalashina wrote:

Fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)
  
  Additions:
• clang-tidy-vs plugin
• Clangd in the intro, the table, and CLion's paragraph

Did you forget to add a new file to the patch?

Please also include full context into the diff. See https://llvm.org/docs/Phabricator.html

Sorry but the new diff was included to the patch.. I can see the updates as Diff 176328. Could you please check and let me know if it does not work? Thank you.

In D54401#1315354, @NoQ wrote:

The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via -checks=-analyzer-....

*summons @alexfh*

In D54945#1316251, @MarinaKalashina wrote:

Fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)
  
  Additions:
• clang-tidy-vs plugin
• Clangd in the intro, the table, and CLion's paragraph

LG in general, but a few comments re: test.

Should this chapter be a separate page linked from index.md?

LG

LG with one more comment. Please address other reviewers' comments though.

Thanks Aaron for volunteering, I'm very thankful for all your work on the reviews, it's much appreciated :D

Great! Thanks for picking this up and pushing forward!

LG

LG with a nit.

error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).

LG in general.

In D53339#1274561, @hwright wrote:

@JonasToth I don't actually have commit privileges, so somebody else will have to commit for me. :)

In D48866#1270361, @hgabii wrote:

Warning messages changed.
Tests updated.
Comments changed based on the recommendations.
Documentation refactored and reformatted.

LG In general, but see a few comments inline.

LG with a nit.

• Remove recursion, use auto.

Looks good with a comment.

In D52684#1250885, @JonasToth wrote:

This patch is related to https://reviews.llvm.org/D51949

To isolate variable declarations (split int * p, v; up) it is necessary to do a lot of work with source location and requires some forward and backwards lexing. The functions there just use the LangOpts and the SourceManager and don't have a ASTContext available, that why I changed this interface.

LG once other comments are addressed.

LG

LG

LG. Thanks

In D52880#1255249, @alexfh wrote:

Have you figured out why exactly does the check hang? Disabling it for -fno-exceptions may just hide a logical problem in the check.

Have you figured out why exactly does the check hang? Disabling it for -fno-exceptions may just hide a logical problem in the check.

In D36836#1249971, @JonasToth wrote:

Anything new here? Is there a LLVM foundation lawyer or something like that we can ask?

LG

So far the change doesn't look really helpful. Are you going to use the function from code where no ASTContext is available? Can you give an example?

LG

In rL343198#289889, @JonasToth wrote:

I commit the patches for CHECK-NOTES assuming the LG to https://reviews.llvm.org/D52230 includes the LG to the question, if i can commit all these patches. If this is not the case, i will of course revert/adjust accordingly.

! In D52334#1242955, @JonasToth wrote:
... to me it makes sense to have clang-tidy without CSA.