[PATCH] D64098: [NFC][clang] Refactor getCompilationPhases step 1: Move list of phases into Types.def table.

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 17 11:57:13 PDT 2019

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
-    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");`

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list