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