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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
