[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+  // Fix the definition.
+  llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
----------------
JonasToth wrote:
> ymandel wrote:
> > JonasToth wrote:
> > > ymandel wrote:
> > > > JonasToth wrote:
> > > > > I feel that this comment doesn't add value. Could you please either make it more expressive or remove it?
> > > > Agreed. I merged the comment below into this one, so one comment describes the rest of the control flow in this block.
> > > Did I tell you the "only one diagnostic in-flight" thing? :D
> > > I told you wrong stuff as you already figured out in the code. Please adjust this comment and the additional scope is not necessary too.
> > But, I think you are *correct* in this assertion.  When I tried multiple in-flight diagnostics, it failed with error:
> > 
> > clang-tidy: /usr/local/google/home/yitzhakm/Projects/llvm/llvm/tools/clang/include/clang/Basic/Diagnostic.h:1297: clang::DiagnosticBuilder clang::DiagnosticsEngine::Report(clang::SourceLocation, unsigned int): Assertion `CurDiagID == std::numeric_limits<unsigned>::max() && "Multiple diagnostics in flight at once!"' failed.
> > 
> > Am I doing something wrong?
> > 
> > 
> Then let me revert what I said and claim the first thing again :D 
> 
> I think, the issue is, that you have the `auto Diag = diag()` object not destroyed before the next one is created. So temporarily storing the locations for the problematic transformations might be necessary to close the scope of the `Diag` first and then emit the notes.
> 
> It would be a good idea, to make a function that returns you a list of FixIts and the list of problematic transformations.
> Having these 2 lists (best is probably `llvm::SmallVector<FixItHint, 4>`, see https://llvm.org/docs/ProgrammersManual.html#dss-smallvector) simplifies creating the diagnostics a lot.
> Then you have 2 scopes for emitting, one scope for the actual warning and replacement and the second scope for emitting the fail-notes.
> 
> These 2 scopes could even be methods (necessary to access `diag()`).
Sounds good. Here's a stab at this restructuring:
https://reviews.llvm.org/differential/diff/169718/

Doesn't seem worth factoring the actual diag() calls into methods, but let me know what you think.

I'll go ahead with other changes as well, just wanted to get this out there...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025