================
Comment at: include/clang/Basic/Module.h:94
@@ +93,3 @@
+ /// the required state of that feature.
+ typedef std::pair<std::string, bool> Requirement;
+
----------------
Daniel Jasper wrote:
> Not really this change, but I'd expect a lot of modules in a single
> compilation to have a similar set of features. Thus, using the
> IdentifierTable to intern these strings and then use their pointer identity
> would be more efficient. Then, you could also represent this using a
> PointerIntPair. It might be the wrong place for optimization, though.
I tried this, but it didn't really seem to make the code cleaner, and I don't
think the memory/disk benefits will be significant, so I backed it out again.
================
Comment at: include/clang/Basic/Module.h:101
@@ -96,3 +100,3 @@
/// available.
- SmallVector<std::string, 2> Requires;
+ SmallVector<Requirement, 2> Requires;
----------------
Daniel Jasper wrote:
> Maybe now rename this to Requirements?
Done.
================
Comment at: include/clang/Basic/Module.h:373
@@ -368,1 +372,3 @@
///
+ /// \param RequiredState The required state of this feature: \c true
+ /// if it must be present, \c false if it must be absent.
----------------
Daniel Jasper wrote:
> I think it is not really a "state", but I also cannot come up with a better
> name. I thought about "IsRequired", but that does not really intuitively mean
> that false means "forbidden".
I could replace it with an `enum`. This whole class needs more encapsulation,
though, and I'd be inclined to do that all as a separate change.
================
Comment at: lib/Basic/Module.cpp:78
@@ -78,1 +77,3 @@
+ Current->Requires[I].second) {
+ Req = Current->Requires[I];
return false;
----------------
Daniel Jasper wrote:
> Probably unimportant: AFAICT, this now copies the string whereas it just
> copied a StringRef before. Maybe one more reason for string interning.
I think we don't care. This code path is only used when issuing a diagnostic.
http://llvm-reviews.chandlerc.com/D2027
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
