[clang] Eliminate TypeProcessingState::trivial.

[mboehme]

Summary

This flag is redundant -- it's true iff savedAttrs is empty.

Querying savedAttrs.empty() should not take any more time than querying the
trivial flag, so this should not have a performance impact either.

I noticed this while working on https://reviews.llvm.org/D111548.

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

168–169

Isn't the same true for this variable? It seems like:

trivial == savedAttrs.empty()
hasSavedAttrs == !savedAttrs.empty()

[mboehme]

clang/lib/Sema/SemaType.cpp

168–169

That's what I also thought at first -- but the situation for hasSavedAttrs is a bit more complicated. It gets set whenever saveDeclAttrs() is called, even if it doesn't actually end up saving any attributes (i.e. if spec.getAttributes() is empty).

hasSavedAttrs is then used to prevent any further calls to saveDeclSpecAttrs() from doing anything:

// Don't try to save them multiple times.
if (hasSavedAttrs) return;

Conceivably, spec _might_ have had attributes added to it in the meantime -- not sure? It might be OK to just replace this logic with if (!savedAttrs.empty()) return; -- but I'm not familiar enough with how this code gets used and therefore decided it would be better not to change it. Can you give additional input on this?

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

168–169

I have the impression that is an oversight in the code rather than an intentional behavior. I think it may be okay to replace the logic with !savedAttrs.empty() as well; if you do that, do you get any test failures? (If you do, then that's a sign something else might be going on.)

[mboehme]

clang/lib/Sema/SemaType.cpp

168–169

I just tried:

// Don't try to save them multiple times.
if (!savedAttrs.empty()) return;

I didn't get any test failures.

Of course, this isn't proof that this is _really_ OK, but it's an indication.

You know this code better than I do, so I would defer to you: How do you think I should proceed? Should I eliminate hasSavedAttrs too?

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

168–169

I think you should eliminate it -- I don't think the behavior here was intentional (based on what I can tell from the code).

[mboehme]

Eliminate hasSavedAttrs as well.

[mboehme]

clang/lib/Sema/SemaType.cpp

168–169

OK -- done!

[aaron.ballman]

LGTM, thank you for the cleanup!

clang/lib/Sema/SemaType.cpp

class TypeProcessingState {
 
     /// The declarator being processed.
     Declarator &declarator;
 
     /// The index of the declarator chunk we're currently processing.
     /// May be the total number of valid chunks, indicating the
     /// DeclSpec.
     unsigned chunkIndex;
 
-    /// Whether there are non-trivial modifications to the decl spec.
-    bool trivial;
-
-    /// Whether we saved the attributes in the decl spec.
-    bool hasSavedAttrs;
-
     /// The original set of attributes on the DeclSpec.
     SmallVector<ParsedAttr *, 2> savedAttrs;
 
     /// A list of attributes to diagnose the uselessness of when the
     /// processing is complete.
     SmallVector<ParsedAttr *, 2> ignoredTypeAttrs;
 
     /// Attributes corresponding to AttributedTypeLocs that we have not yet
     /// populated.
class TypeProcessingState {
 
     /// Flag to indicate we parsed a noderef attribute. This is used for
     /// validating that noderef was used on a pointer or array.
     bool parsedNoDeref;
 
   public:
     TypeProcessingState(Sema &sema, Declarator &declarator)
         : sema(sema), declarator(declarator),
-          chunkIndex(declarator.getNumTypeObjects()), trivial(true),
-          hasSavedAttrs(false), parsedNoDeref(false) {}
+          chunkIndex(declarator.getNumTypeObjects()), parsedNoDeref(false) {}
 
     Sema &getSema() const {
       return sema;
     }
 
     Declarator &getDeclarator() const {
       return declarator;
     }
ParsedAttributesView &getCurrentAttributes() const {
       if (isProcessingDeclSpec())
         return getMutableDeclSpec().getAttributes();
       return declarator.getTypeObject(chunkIndex).getAttrs();
     }
 
     /// Save the current set of attributes on the DeclSpec.
     void saveDeclSpecAttrs() {
       // Don't try to save them multiple times.
-      if (hasSavedAttrs) return;
+      if (!savedAttrs.empty())
+        return;
 
       DeclSpec &spec = getMutableDeclSpec();
       llvm::append_range(savedAttrs,
                          llvm::make_pointer_range(spec.getAttributes()));
-      trivial &= savedAttrs.empty();
-      hasSavedAttrs = true;
     }
 
     /// Record that we had nowhere to put the given type attribute.
     /// We will diagnose such attributes later.
     void addIgnoredTypeAttr(ParsedAttr &attr) {
       ignoredTypeAttrs.push_back(&attr);
     }
 
void setExpansionLocForMacroQualifiedType(const MacroQualifiedType *MQT,
       LocsForMacros[MQT] = Loc;
     }
 
     void setParsedNoDeref(bool parsed) { parsedNoDeref = parsed; }
 
     bool didParseNoDeref() const { return parsedNoDeref; }
 
     ~TypeProcessingState() {
-      if (trivial) return;
+      if (savedAttrs.empty())
+        return;
 
-      restoreDeclSpecAttrs();
+      getMutableDeclSpec().getAttributes().clearListOnly();
+      for (ParsedAttr *AL : savedAttrs)
+        getMutableDeclSpec().getAttributes().addAtEnd(AL);
     }
 
   private:
     DeclSpec &getMutableDeclSpec() const {
       return const_cast<DeclSpec&>(declarator.getDeclSpec());
     }
-
-    void restoreDeclSpecAttrs() {
-      assert(hasSavedAttrs);
-
-      getMutableDeclSpec().getAttributes().clearListOnly();
-      for (ParsedAttr *AL : savedAttrs)
-        getMutableDeclSpec().getAttributes().addAtEnd(AL);
-    }
   };
 } // end anonymous namespace
 
 static void moveAttrFromListToList(ParsedAttr &attr,
                                    ParsedAttributesView &fromList,
                                    ParsedAttributesView &toList) {
   fromList.remove(&attr);
   toList.addAtEnd(&attr);