[PATCH] D141892: Implement modernize-use-constraints

[Nathan James via Phabricator via cfe-commits]

njames93 added a reviewer: carlosgalvezp.
njames93 added a comment.

Would you consider supporting enable_if via parameters

  template<typename T>
  void doStuff(T&, std::enable_if_t<T::some_value, void*> = nullptr) {}



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:71
+
+    return std::make_optional(Specialization);
+  }
----------------
nit: Don't need to explicitly call make_optional here, the implicit conversion will handle that for you, same goes for everywhere else.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:89
+
+    if (const auto *AliasedType = llvm::dyn_cast<DependentNameType>(
+            Specialization.getTypePtr()->getAliasedType())) {
----------------
`dyn_cast` is brought into clangs namespace so the `llvm::` qualifier is unnecessary here


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:109-112
+  std::optional<TemplateSpecializationTypeLoc> EnableIf;
+  EnableIf = matchEnableIfSpecializationImplTypename(TheType);
+  if (EnableIf)
+    return EnableIf;
----------------
Nit


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:125-128
+  std::optional<TemplateSpecializationTypeLoc> EnableIf =
+      matchEnableIfSpecializationImpl(TheType);
+  if (EnableIf)
+    return std::make_optional(EnableIfData{std::move(*EnableIf), TheType});
----------------
Ditto as above


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:129-130
+    return std::make_optional(EnableIfData{std::move(*EnableIf), TheType});
+  else
+    return std::nullopt;
+}
----------------
Don't use else after a return.
Same goes for anywhere else that this pattern occurs


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:133
+
+static std::tuple<std::optional<EnableIfData>, const Decl *>
+matchTrailingTemplateParam(const FunctionTemplateDecl *FunctionTemplate) {
----------------
Prefer pair over tuple when only 2 elements


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:157-160
+    return std::make_tuple(
+        matchEnableIfSpecialization(
+            LastTemplateParam->getTypeSourceInfo()->getTypeLoc()),
+        LastTemplateParam);
----------------
Prefer braced initialization rather than the factory make_tuple(or make_pair). Same goes below.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:215
+
+static std::optional<std::string>
+getTypeText(ASTContext &Context,
----------------
This can return a std::optional<StringRef> if you remove the call the `.str()` and assignment to `std::string` below.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:266
+
+static Token getPreviousToken(SourceLocation &Location, const SourceManager &SM,
+                              const LangOptions &LangOpts,
----------------
Isn't there a copy of this function in `clang::tidy::utils`?


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:296-299
+  if (llvm::dyn_cast<ParenExpr>(Expression))
+    return true;
+  if (llvm::dyn_cast<DependentScopeDeclRefExpr>(Expression))
+    return true;
----------------



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:335
+  if (EndsWithDoubleSlash)
+    return std::make_optional(AddParens(ConditionText.str()));
+  else
----------------
No need to call `.str()` here if the lambda expects a `StringRef`, also removed `std::make_optional`.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:459-461
+  std::optional<EnableIfData> EnableIf;
+  EnableIf = matchEnableIfSpecialization(*ReturnType);
+  if (EnableIf.has_value()) {
----------------



================
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;
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:471-474
+  const Decl *LastTemplateParam = nullptr;
+  std::tie(EnableIf, LastTemplateParam) =
+      matchTrailingTemplateParam(FunctionTemplate);
+  if (EnableIf.has_value() && LastTemplateParam) {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141892