aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D75041#2799036 <https://reviews.llvm.org/D75041#2799036>, @whisperity wrote: > In D75041#2799023 <https://reviews.llvm.org/D75041#2799023>, @martong wrote: > >> Perhaps all conversion related logic should go into their own implementation >> file? Seems like it adds up to roughly 1000 lines. > > That's against the usual conventions of the project (1 TU for 1 check > implementation). The methods are grouped by namespace, you can fold it up in > your editor. FWIW, there's not a hard rule for that -- we put other tidy logic into Utils files, for example. However, I think such a cleanup could be done post-commit as an NFC change as well. It's taken me a while to convince myself that it's reasonable to reimplement so much of the conversion logic in clang-tidy, but I don't think it's reasonable to try to expose an API from Clang for its conversion logic to be reusable in this context. So despite my mild discomfort with how complex the logic is in this patch, I think it's an acceptable approach. LGTM, aside from some minor style nits. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:249 + /// the conversion sequence. This method does **NOT** return Begin and End. + SmallVector<QualType, 4> getInvolvedTypesInSequence() const { + SmallVector<QualType, 4> Ret; ---------------- Return a `SmallVectorImpl<QualType>` so that the size of the vector doesn't matter to callers? ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:556 +static inline bool isUselessSugar(const Type *T) { + return isa<DecayedType, ElaboratedType, ParenType>(T); +} ---------------- One thing that could get interesting here are attributed types, as those may be "useless" or "useful" sugars depending on the attribute. However, I think we can deal with that later when we have more real-world uses in front of us. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:962 +public: + /// The conversion assocaited with a conversion function, together with the + /// mixability flags of the conversion function's parameter or return type ---------------- ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1491 + + const auto &AddType = [&](StringRef ToAdd) { + if (LastAddedType != ToAdd && ToAdd != SeqEndTypeStr) { ---------------- ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1493 + if (LastAddedType != ToAdd && ToAdd != SeqEndTypeStr) { + OS << " -> '" << ToAdd << '\''; + LastAddedType = ToAdd.str(); ---------------- ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1503 + if (LastAddedType != DestinationTypeAsDiagnosed) { + OS << " -> '" << DestinationTypeAsDiagnosed << '\''; + LastAddedType = DestinationTypeAsDiagnosed.str(); ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75041/new/ https://reviews.llvm.org/D75041 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
