In D50699#1198813, @owenpan wrote:

Updated patch created by "svn diff --diff-cmd=diff -x -U999999"

Please create the diffs with full context (git diff -U 9999999) and add an test-case.

Using git for the diff, add testcase

Please add a test for the same usecase as in Sema.

Thank you very much :)

• update to current master of clang introduce CHECK-NOTES
• use new BeginLoc api

• explicitly ignore lambdas in VarDecls

Could it happen that some template specializations or so need to land in absl?

Could there even be a false positive? In a sense, the variable is never
used, so it does not matter, not?

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

No concerns except the small nits from my side.

Thank you for your first contribution to LLVM btw :)

LGTM with only the formatting question.

LGTM

Do you think it is a bad idea? If the variable is not used it is ok to
ignore it in this particular circumstance. Other warnings/check should
deal with such a situation IMHO.

LGTM, but alex or aaron should allow the commit ;)

• fix bug with AnalyzeValues==false skipping whole check, adjust test code to 'const' instead of const

Given it is about the performance in loops and the optimizer?! can
better work with the copy-and-don't-use it might make sense to just
check if the variable is dereferenced in the scope of the loop (any
declRefExpr exists).

@alexfh or @aaron.ballman could you take a final look on this one?

Is the type important, or specifics about the variable (e.g. the name?)
The _ variable names are sometimes used for RAII variables/lambdas
that shall just do some cleanup, e.g. gsl::finally().

If whitelisting works, thats fine. But I agree with @lebedev.ri that a contribution/improvement to the ExprMutAnalyzer is the better option. This is especially the case, because multiple checks (and maybe even some other parts then clang-tidy) will utilize this analysis.

Could you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

For reference, here is the output for llvm/lib.

No nothing. I think it is barely noticable in general. From prior long
runs i notices the scrambled messages, but it was only a small fraction.
So most of the time the threads dont seem to interfere, which makes the
lock kinda low-cost.

I do run it over llvm/lib, there is no visible effect. from my experience maybe 1-5% of the lines are interleafed without the patch, too.

Hi deannagarcia and thank you for your first contribution :)

Lets say running it over whole LLVM for one check? I will apply this path locally, because i have to analyze llvm right now anyway.

I forgot: Did you measure how much of a time penalty this brings? Just out of curiosity, correct script is more important then fast script :)

I would like to have this patch in, because i currently have a 1MB output file, and the missing synchronization really destroys cleaning with grep and sed :(

In D45444#1189262, @aaron.ballman wrote:

However, I'm wondering how this should integrate with other const-correctness efforts like readability-non-const-parameter?

• address review issues, todos/fixmes and diag nit

@hokein Do you think this should go into the 7.0 release?

• rebase

• update ReleaseNotes

• fix typo

• revert breaking change in LocalVar matcher.

• doc list

• Merge branch 'master' into check_const
• [Misc] rename and first review comments
• language stuff

I'm sorry for the delay in reviewing this; I'm not certain how it fell off my radar for so long!

• remove double blank line

• add better diagnostics about template instantiated exception types

@lebedev.ri and @alexfh i would change the tests in https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this one?

• get check up to date

get check up to 8.0

I think clang/lib/Analysis is a good place.

I think it should in theory be possible, though I need some advice about where exactly is the best place in clang.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

correct rebase.

• Merge branch 'master' into check_const

rebase to master

rebase to master

• rebase

I reordered the anchoring list to be in alphabetical order as well!

In D49114#1182071, @0x8000-0000 wrote:

The state of this patch:

• user interface is as agreed-upon, giving end-users the capability to filter out floats and ints as it makes sense for their code base
• code is clean
• documentation is up to date
• default ignore lists are sensible
  
  There is one outstanding improvement request - to combine the two float/double lists into a single APFloat but that requires more refactoring outside this check and the benefit lies mainly in a future extensibility and the saving of a couple hundred bytes.
  
  @aaron.ballman - can we get this merged as-is and improve it later? I'd like to see this check available in Clang7.

Actually, maybe this patch is relevant for clang-tooling itself (where the compilation-database stuff actually is). @alexfh what is your opinion on that?

In D49890#1178103, @lebedev.ri wrote:

I'm not sure what this differential *actually* does.

@aaron.ballman Are you missing something in this check/tests? I think i will remove the big comment with my thoughts on what has to be done, or do you think it would help for reference?

I did not find any major issue :)

rebase