This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by plotfi on Jul 2 2019, 1:48 PM.

Diff Detail

Event Timeline

plotfi created this revision.Jul 2 2019, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 1:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

compnerd added inline comments.Jul 2 2019, 2:22 PM
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
};
plotfi updated this revision to Diff 207659.Jul 2 2019, 4:40 PM
plotfi marked 4 inline comments as done.

Update diff based on @compnerd's feedback

plotfi marked an inline comment as done.Jul 2 2019, 4:40 PM

@aaron.ballman Any thoughts on this?

aaron.ballman added inline comments.Jul 17 2019, 11:57 AM
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:
assert(std::equal(Phases.begin(), Phases.end(), P.begin(), P.end()) && "Invalid phase or size");

plotfi updated this revision to Diff 210431.Jul 17 2019, 2:56 PM
plotfi marked 10 inline comments as done.

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!

aaron.ballman added inline comments.Jul 18 2019, 12:01 AM
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;

plotfi marked an inline comment as done.Jul 18 2019, 9:38 AM
plotfi added inline comments.
clang/lib/Driver/Types.cpp
306

Oh yeah sure. I can change that. Anything else pop out at you?

plotfi marked an inline comment as done.Jul 18 2019, 9:47 AM
aaron.ballman added inline comments.Jul 18 2019, 11:13 AM
clang/lib/Driver/Types.cpp
306

You can drop the P.clear() as part of that assignment, but otherwise, no.

plotfi updated this revision to Diff 210696.Jul 18 2019, 3:56 PM
plotfi marked 2 inline comments as done.
plotfi added inline comments.
clang/lib/Driver/Types.cpp
306

Ahh. I understand what you mean now. Thanks!

aaron.ballman accepted this revision.Jul 19 2019, 12:07 AM

LGTM!

clang/lib/Driver/Types.cpp
304

Spurious a at the end of the comment should be removed.

This revision is now accepted and ready to land.Jul 19 2019, 12:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 4:12 PM
plotfi updated this revision to Diff 211651.Jul 24 2019, 6:03 PM
plotfi marked an inline comment as done.

Removing 'A'

This was a mistake. Updated the Wrong diff.