[PATCH] [lld] Proposal and example for changing Atom attributes.

[kledzik at apple.com]

================
Comment at: include/lld/Core/DefinedAtom.h:233
@@ +232,3 @@
+  /// \brief Alter the visibility of this atom.
+  virtual void setScope(Scope);
+
----------------
Rui Ueyama wrote:
> I assume we will add more setXX member functions (e.g. setInterposable) when we need it. I don't like to add them now but want to make sure that we are on the same page.
Yes.  I alluded to that in the overview, that the ELF could add setExportDynamic()

================
Comment at: include/lld/Core/LinkingContext.h:230
@@ +229,3 @@
+  /// as adjusting the atom's scope.
+  virtual void notifySymbolTableAdd(const Atom *atom) const {
+  }
----------------
Rui Ueyama wrote:
> This function signature made me a bit sad because it's intended to mutate a given atom despite it takes const Atom *. We may probably want to make the atom graph mutable in the first place, because it's no longer actually const because of the addition of this function, so that we don't have to sprinkle const_casts in many places. I don't like to see that change in this CL but it may need to be considered later.
Yes. For now the graph is 99.9% const, so a few const_cast<> keeps it closer to what we are trying to model.  But if we have Passes that need to modify atoms, or lots of places need start modifying atoms, it will probably make sense to remove the const in the zillion places.

================
Comment at: lib/Core/DefinedAtom.cpp:87
@@ +86,3 @@
+void DefinedAtom::setScope(Scope s) {
+  assert(s == scope() && "setScope() must be overridden to alter scope");
+}
----------------
Rui Ueyama wrote:
> Does this have to be compared with the current value? I don't think we want to call setScope() if it's not really implemented. It seems to me that we can just put llvm_unreachable here.
I started with that, but thought it would be odd to case an abort if something called setScope() and was not actually changing the scope.  I'll change it back.

================
Comment at: lib/ReaderWriter/MachO/MachOLinkingContext.cpp:585
@@ +584,3 @@
+
+  if (currentlyGlobal != whiteListed) {
+    if (currentlyGlobal) {
----------------
Rui Ueyama wrote:
> You can early return here if you invert the condition inside if.
yes!

================
Comment at: lib/ReaderWriter/MachO/MachOLinkingContext.cpp:602
@@ +601,3 @@
+  if (currentlyGlobal == blackListed) {
+    if (currentlyGlobal) {
+      // Reduce scope of symbols in unexport list down to "hidden".
----------------
Rui Ueyama wrote:
> if (currentlyGlobal && currentlyGlobal == blacklisted)?
It reduces to:

   if (currentlyGlobal && blacklisted)

http://reviews.llvm.org/D4965