aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Driver/Types.h:100 /// done for type 'Id'. - void getCompilationPhases( - ID Id, - llvm::SmallVectorImpl<phases::ID> &Phases); + const std::vector<phases::ID> getCompilationPhases(ID Id); ---------------- Please drop the top-level `const` as it doesn't add much utility. ================ Comment at: clang/lib/Driver/Driver.cpp:2217 public: - typedef llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> PhasesTy; + typedef const std::vector<phases::ID> PhasesTy; ---------------- Why are you changing this to an STL type? ================ Comment at: clang/lib/Driver/Driver.cpp:3236 - PL.clear(); - types::getCompilationPhases(InputType, PL); + const std::vector<phases::ID> PL = types::getCompilationPhases(InputType); + LastPLSize = PL.size(); ---------------- You could use a `const &` here and avoid the copy. Either that, or drop the `const` (it's not a common pattern in the code base). ================ Comment at: clang/lib/Driver/Driver.cpp:3278 const types::ID HeaderType = lookupHeaderTypeForSourceType(InputType); - llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> PCHPL; - types::getCompilationPhases(HeaderType, PCHPL); + const std::vector<phases::ID> PCHPL = + types::getCompilationPhases(HeaderType); ---------------- Same here ================ Comment at: clang/lib/Driver/Types.cpp:271 +// the old behavior and a subsequent change will delete most of the body. +const llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> &types::getCompilationPhases(ID Id) { + llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> P; ---------------- compnerd wrote: > Can we return `llvm::SmallVectorImpl<phases::ID>` instead? The size is > immaterial to this I think. Agreed, I think this API should be working with `SmallVector`s and not `std::vector`s. ================ Comment at: clang/lib/Driver/Types.cpp:300 + // in a subsequent NFC commit. + auto Phases = getInfo(Id).Phases; + assert(Phases.size() == P.size() && "Invalid size."); ---------------- Please don't use `auto` here as the type is not spelled out in the initialization. ================ Comment at: clang/lib/Driver/Types.cpp:301-303 + assert(Phases.size() == P.size() && "Invalid size."); + for (unsigned i = 0; i < Phases.size(); i++) + assert(Phases[i] == P[i] && "Invalid Phase"); ---------------- You can replace all three lines by: `assert(std::equal(Phases.begin(), Phases.end(), P.begin(), P.end()) && "Invalid phase or size");` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64098/new/ https://reviews.llvm.org/D64098 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits