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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64
+
+  // Fix the definition.
+  llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
----------------
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()`).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025