[PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.

[Peter Collingbourne via cfe-commits]

pcc added inline comments.

================
Comment at: docs/ClassScope.rst:2
@@ +1,3 @@
+===========
+Class Scope
+===========
----------------
rsmith wrote:
> Can you use some word other than "scope" here? "Class scope" is already a term of art in C++, meaning something completely different. I think what you're referring to is exactly the visibility of the class (in the ELF sense).
Yes, this is pretty much visibility. I wanted to avoid using the term "visibility" because I'm introducing flags and attributes which can make scope mean something different to object file visibility, so I wanted to avoid the overload to avoid confusion.

Maybe the overloading would help with understanding though if I add a qualifying adjective. This is all about whether all derived classes are visible, so maybe the right term is something like "derived visibility"?

================
Comment at: docs/ClassScope.rst:13
@@ +12,3 @@
+to a single linkage unit (i.e. the executable or DSO). It is effectively an
+ODR violation to declare classes with linkage-unit scope in multiple linkage
+units.  Classes with global scope may be defined in multiple linkage units,
----------------
rsmith wrote:
> classes -> a class
> 
> Otherwise this reads like you're saying at most one linkage unit per program can declare classes with linkage-unit scope.
Will do

================
Comment at: docs/ClassScope.rst:14
@@ +13,3 @@
+ODR violation to declare classes with linkage-unit scope in multiple linkage
+units.  Classes with global scope may be defined in multiple linkage units,
+but the tradeoff is that the virtual function call optimization and control
----------------
rsmith wrote:
> Classes -> A class
Will do

================
Comment at: docs/ClassScope.rst:23
@@ +22,3 @@
+
+  -  ``-fdefault-class-scope=attrs`` indicates that the compiler will infer
+     class scope based on platform-specific attributes that control the class's
----------------
eugenis wrote:
> Maybe call it "default"? Attrs sounds too specific. Basically this setting lets clang figure out scope based on the source code.
`-fdefault-class-scope=default`? Sounds a little stuttery. Although `attrs` isn't entirely accurate (it's also affected by flags), it's close enough I reckon.

================
Comment at: docs/ClassScope.rst:28
@@ +27,3 @@
+     or the ``-fvisibility=hidden -fvisibility-inlines-hidden`` flags)
+     receive global scope, and all others receive linkage-unit scope. When
+     targeting Windows, classes with the ``__declspec(dllexport)`` or
----------------
eugenis wrote:
> hidden visibility = linkage-unit scope, not global scope.
Yes, I somehow completely screwed up the sense of these, even in the Windows part. Will fix.


http://reviews.llvm.org/D18635