Simplifying the phases generation process, first by copying the phases info into the Table.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The explicit list I think is way better for readability, this is a nice starting point for cleaning this up.
| clang/include/clang/Driver/Types.def | ||
|---|---|---|
| 18 | Please update the comment here indicating the new field | |
| clang/lib/Driver/Driver.cpp | ||
| 3302–3303 | Why can this not be a range based for loop? | |
| clang/lib/Driver/Types.cpp | ||
| 271 | Can we return llvm::SmallVectorImpl<phases::ID> instead? The size is immaterial to this I think. | |
| 305 | A comment to indicate that the concrete list will become the implementation would be nice. | |
| clang/lib/Driver/Types.cpp | ||
|---|---|---|
| 30 | I think that we can abuse the preprocessor a bit and get something that is nicer to read. Lets make the last parameter a variadic and pass along the phases to the list. Something like: ENTRY('a', "std::string")
ENTRY('b', "std::string", "std::vector<std::string>")
ENTRY('c', "std::string", "std::vector<std::string>", "int")
const struct {
unsigned int id;
std::vector<std::string> strings;
} array[] = {
#define ENTRY(id, strings...) { id, { strings } },
#include "reduced.def"
#undef ENTRY
}; | |
| clang/include/clang/Driver/Types.h | ||
|---|---|---|
| 100 | Please drop the top-level const as it doesn't add much utility. | |
| clang/lib/Driver/Driver.cpp | ||
| 2217 | Why are you changing this to an STL type? | |
| 3236 | 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). | |
| 3278 | Same here | |
| clang/lib/Driver/Types.cpp | ||
| 271 | Agreed, I think this API should be working with SmallVectors and not std::vectors. | |
| 300 | Please don't use auto here as the type is not spelled out in the initialization. | |
| 301–303 | You can replace all three lines by: | |
Updated to address @aaron.ballman 's feedback
| clang/lib/Driver/Driver.cpp | ||
|---|---|---|
| 2217 | I changed it because it didn't look like you can return a SmallVector, but I just changed it back to the old SmallVector function calling style. | |
| 3236 | Going to keep the copy. I want to eventually merge getCompilationPhases and getFinalPhase into the same function. | |
| clang/lib/Driver/Types.cpp | ||
| 301–303 | Nice! | |
| clang/lib/Driver/Types.cpp | ||
|---|---|---|
| 306 | Can't you use the local Phases object instead of calling getInfo() again? This seems like it wants to be P = Phases; | |
| clang/lib/Driver/Types.cpp | ||
|---|---|---|
| 306 | Oh yeah sure. I can change that. Anything else pop out at you? | |
| clang/lib/Driver/Types.cpp | ||
|---|---|---|
| 306 | You can drop the P.clear() as part of that assignment, but otherwise, no. | |
| clang/lib/Driver/Types.cpp | ||
|---|---|---|
| 306 | Ahh. I understand what you mean now. Thanks! | |
LGTM!
| clang/lib/Driver/Types.cpp | ||
|---|---|---|
| 304 | Spurious a at the end of the comment should be removed. | |
Please drop the top-level const as it doesn't add much utility.