MC: Remove redundant `SetUsed` arguments in MCSymbol methods. NFC

[sbc100]

Summary

Only AsmParser.cpp seems to care about isUsed and it only overrides SetUsed
for isUndefined and getVariableValue.

Also setUsed() is never called, so remove it.

[vsk]

Your changes to lib/ lgtm.

IMO getting rid of the SetUsed parameters entirely would be preferable to having only some predicates accept SetUsed. Have you considered removing the update to IsUsed in getVariableValue(), and inserting calls to setUsed() as needed to fix any regressions?

[sbc100]

In D41971#974111, @vsk wrote:

Your changes to lib/ lgtm.

IMO getting rid of the SetUsed parameters entirely would be preferable to having only some predicates accept SetUsed. Have you considered removing the update to IsUsed in getVariableValue(), and inserting calls to setUsed() as needed to fix any regressions?

I think you are probably right, but I didn't want to change AsmParser.cpp too much because I don't know that part of the codebase at all. This change made sense to me because I can easily see that it is a NFC. Are you suggesting that AsmParser.cpp rely on calling setUsed explicitly rather than implicitly? I think that would probably be more clear.

[vsk]

In D41971#974113, @sbc100 wrote:

In D41971#974111, @vsk wrote:

Your changes to lib/ lgtm.

IMO getting rid of the SetUsed parameters entirely would be preferable to having only some predicates accept SetUsed. Have you considered removing the update to IsUsed in getVariableValue(), and inserting calls to setUsed() as needed to fix any regressions?

I think you are probably right, but I didn't want to change AsmParser.cpp too much because I don't know that part of the codebase at all. This change made sense to me because I can easily see that it is a NFC. Are you suggesting that AsmParser.cpp rely on calling setUsed explicitly rather than implicitly? I think that would probably be more clear.

I also don't really know this part of the codebase, and only added the default parameters to fix a bug without causing excessive churn. I do think it would be clearer if setUsed is called explicitly as needed. If you think that's the right approach but don't have time to look into it, I'll volunteer to take a stab at it.

[sbc100]

I can take a look.

[• espindola]

LGTM.

A change removing more uses of setUsed=true would be nice, but it can be a followup.

include/llvm/MC/MCSymbol.h

private:
   void operator delete(void*, unsigned) {
     llvm_unreachable("Constructor throws?");
   }
   /// \brief Placement delete - required by std, but never called.
   void operator delete(void*, unsigned, bool) {
     llvm_unreachable("Constructor throws?");
   }
 
-  MCSection *getSectionPtr(bool SetUsed = true) const {
-    if (MCFragment *F = getFragment(SetUsed)) {
+  MCSection *getSectionPtr() const {
+    if (MCFragment *F = getFragment()) {
       assert(F != AbsolutePseudoFragment);
       return F->getParent();
     }
     return nullptr;
   }
 
   /// \brief Get a reference to the name field.  Requires that we have a name
   const StringMapEntry<bool> *&getNameEntryPtr() {
public:
   /// \name Accessors
   /// @{
 
   /// isTemporary - Check if this is an assembler temporary symbol.
   bool isTemporary() const { return IsTemporary; }
 
   /// isUsed - Check if this is used.
   bool isUsed() const { return IsUsed; }
-  void setUsed(bool Value) const { IsUsed |= Value; }
 
   /// \brief Check if this symbol is redefinable.
   bool isRedefinable() const { return IsRedefinable; }
   /// \brief Mark this symbol as redefinable.
   void setRedefinable(bool Value) { IsRedefinable = Value; }
   /// \brief Prepare this symbol to be redefined.
   void redefineIfPossible() {
     if (IsRedefinable) {
       if (SymbolContents == SymContentsVariable) {
         Value = nullptr;
         SymbolContents = SymContentsUnset;
       }
       setUndefined();
       IsRedefinable = false;
     }
   }
 
   /// @}
   /// \name Associated Sections
   /// @{
 
   /// isDefined - Check if this symbol is defined (i.e., it has an address).
   ///
   /// Defined symbols are either absolute or in some section.
-  bool isDefined(bool SetUsed = true) const {
-    return getFragment(SetUsed) != nullptr;
-  }
+  bool isDefined() const { return !isUndefined(); }
 
   /// isInSection - Check if this symbol is defined in some section (i.e., it
   /// is defined but not absolute).
-  bool isInSection(bool SetUsed = true) const {
-    return isDefined(SetUsed) && !isAbsolute(SetUsed);
+  bool isInSection() const {
+    return isDefined() && !isAbsolute();
   }
 
   /// isUndefined - Check if this symbol undefined (i.e., implicitly defined).
-  bool isUndefined(bool SetUsed = true) const { return !isDefined(SetUsed); }
+  bool isUndefined(bool SetUsed = true) const {
+    return getFragment(SetUsed) == nullptr;
+  }
 
   /// isAbsolute - Check if this is an absolute symbol.
-  bool isAbsolute(bool SetUsed = true) const {
-    return getFragment(SetUsed) == AbsolutePseudoFragment;
+  bool isAbsolute() const {
+    return getFragment() == AbsolutePseudoFragment;
   }
 
   /// Get the section associated with a defined, non-absolute symbol.
-  MCSection &getSection(bool SetUsed = true) const {
-    assert(isInSection(SetUsed) && "Invalid accessor!");
-    return *getSectionPtr(SetUsed);
+  MCSection &getSection() const {
+    assert(isInSection() && "Invalid accessor!");
+    return *getSectionPtr();
   }
 
   /// Mark the symbol as defined in the fragment \p F.
   void setFragment(MCFragment *F) const {
     assert(!isVariable() && "Cannot set fragment of variable");
     FragmentAndHasName.setPointer(F);
   }
 

lib/MC/MCCodeView.cpp

void CodeViewContext::encodeInlineLineTable(MCAsmLayout &Layout,
 
   unsigned EndSymLength =
       computeLabelDiff(Layout, LastLabel, Frag.getFnEndSym());
   unsigned LocAfterLength = ~0U;
   ArrayRef<MCCVLineEntry> LocAfter = getLinesForExtent(LocEnd, LocEnd + 1);
   if (!LocAfter.empty()) {
     // Only try to compute this difference if we're in the same section.
     const MCCVLineEntry &Loc = LocAfter[0];
-    if (&Loc.getLabel()->getSection(false) == &LastLabel->getSection(false))
+    if (&Loc.getLabel()->getSection() == &LastLabel->getSection())
       LocAfterLength = computeLabelDiff(Layout, LastLabel, Loc.getLabel());
   }
 
   compressAnnotation(BinaryAnnotationsOpCode::ChangeCodeLength, Buffer);
   compressAnnotation(std::min(EndSymLength, LocAfterLength), Buffer);
 }
 
 void CodeViewContext::encodeDefRange(MCAsmLayout &Layout,

lib/Target/Hexagon/MCTargetDesc/HexagonMCELFStreamer.cpp

StringRef SectionName =
         ((AccessSize == 0) || (Size == 0) || (Size > GPSize))
             ? ".bss"
             : sbss[(Log2_64(AccessSize))];
     MCSection &Section = *getAssembler().getContext().getELFSection(
         SectionName, ELF::SHT_NOBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
     MCSectionSubPair P = getCurrentSection();
     SwitchSection(&Section);
 
-    if (ELFSymbol->isUndefined(false)) {
+    if (ELFSymbol->isUndefined()) {
       EmitValueToAlignment(ByteAlignment, 0, 1, 0);
       EmitLabel(Symbol);
       EmitZeros(Size);
     }
 
     // Update the maximum alignment of the section if necessary.
     if (ByteAlignment > Section.getAlignment())
       Section.setAlignment(ByteAlignment);