[PATCH] D23552: [ELF] - Give automatically generated __start_* and __stop_* symbols default visibility.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/SymbolTable.cpp:425
@@ -424,3 +424,3 @@
   std::tie(S, WasInserted) =
-      insert(N, STT_NOTYPE, STV_HIDDEN, /*CanOmitFromDynSym*/ false,
+      insert(N, STT_NOTYPE, StOther & 3, /*CanOmitFromDynSym*/ false,
              /*IsUsedInRegularObj*/ true, nullptr);
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > I'm not the type of person who wants to eliminate all constants, but 3? It's too magical.
> > Actually it is used in many places in lld. I count 4 more places in this file (SymbolTable.cpp) and also once in Symbols.h:
> > 
> > ```
> > uint8_t getVisibility() const { return StOther & 0x3; }
> > ```
> > 
> > I suggest to leave it for now and fix once for all places if you want it.
> The use of `0x3` in `getVisibility` is fine because it is obvious that it is extracting visibility bits. It actually hides the use of `0x3` and wraps it with a better name (getVisibility). But in this context there's no information as to what you are doing.
What about adding comment then ?

```
  bool WasInserted;
  std::tie(S, WasInserted) =
      insert(N, STT_NOTYPE, StOther & 0x3 /*Visibility*/, /*CanOmitFromDynSym*/ false,
```

Or do you prefer static method for that ?


```
static unsigned getVisibility(unsigned O) { 
  return O & 0x3;
}

...

  std::tie(S, WasInserted) =
      insert(N, STT_NOTYPE, getVisibility(StOther), /*CanOmitFromDynSym*/ false,
```


https://reviews.llvm.org/D23552