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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
