[PATCH] D141892: Implement modernize-use-constraints

[Chris Cotter via Phabricator via cfe-commits]

ccotter added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:157-160
+    return std::make_tuple(
+        matchEnableIfSpecialization(
+            LastTemplateParam->getTypeSourceInfo()->getTypeLoc()),
+        LastTemplateParam);
----------------
njames93 wrote:
> Prefer braced initialization rather than the factory make_tuple(or make_pair). Same goes below.
I've seen this feedback twice now - is this (or others, like preferring tuple over pair) written down somewhere? I didn't see anything on https://llvm.org/docs/CodingStandards.htm (or it wasn't obvious to me). Thanks!


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:266
+
+static Token getPreviousToken(SourceLocation &Location, const SourceManager &SM,
+                              const LangOptions &LangOpts,
----------------
njames93 wrote:
> Isn't there a copy of this function in `clang::tidy::utils`?
Ah, this version that I copied takes the `Location` by modifiable reference so it can be updated. I will update the original version


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:462-466
+    std::vector<FixItHint> FixIts =
+        handleReturnType(Function, *ReturnType, *EnableIf, *Result.Context);
+    diag(ReturnType->getBeginLoc(),
+         "use C++20 requires constraints instead of enable_if")
+        << FixIts;
----------------
njames93 wrote:
> Though a different approach of passing the `DiagnosticBuilder` to the `handleReturnType` function and just appending the fixits in place would be a nicer solution.
> Maybe change the function name to `addReturnTypeFixes(DiagnosticBuilder&, const FunctionDecl *, const TypeLoc &, const EnableIfData &, ASTContext &);`
> 
> Same transformation can be made below.
Updated to apply your first suggestion.

For the `addReturnTypeFixes` suggestion, I find that `handleReturnType` returning the `vector` of `FixItHints` simpler since it has less responsibility or knowledge about how the `FixItHints` are to be applied - it's main purpose is to compute them, so why have the function also know how to apply them? Practically speaking, maybe this is not really useful since this function is not say, exposed as a library function to other code (it's just used as an implementation detail of this check).

Can you clarify what makes the application in place a nicer solution? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141892/new/

https://reviews.llvm.org/D141892