Make meanings of variables clearer in action table generation (NFC)

[kristina]

IMO these comments don't really provide anything that isn't clear from names of the variables ie. SizeTypeID: size of typeid (usually 1). In fact statements like "usually 1" provide more ambiguity as opposed to clarifying anything. Same goes for SizeActions: size of all action entries for a function, the variable names are descriptive enough.

[aheejin]

Yes.. I agree 'usually something' does not sound great. I added that because I thought it would be rare for those variables to have size other than 1 or 2 using LEB128 encoding, but I can remove it.
I also agree the meanings of SizeTypeID and SizeActions are easy to guess from their variable names, but to me the code was a bit confusing because there were also SizeSiteActions and SizeAction too. I think if we had these comments that would have saved me at least 10 mins I guess, so I added them. Do you think all of the comments here are very obvious and not necessary?

[kristina]

I don't think any of them are necessary, especially saying "usually 1" without any further explanation. They could be less ambiguous with an elaboration like "usually 1 unless ... etc", but all in all, I don't think they're helpful as they basically just repeat the variable names.

[kristina]

If you really think it would help, I would suggest renaming that variable but still keep the name brief but more informative. That's a better solution that polluting it with comments, but I'd still lean towards not changing anything at all, since I don't think it's that confusing, and the developer policy generally leans against such changes unless they really vastly improve code clarity, which is not the case here from what I feel.

[aheejin]

I simplified the comments a little. I don't think LLVM developer policy is
against simple-ish comments, as long as they are relevant and they help
understanding. I also don't think adding simple comments are 'polluting' the
code. The code logic and variable meanings here might seem obvious to some
people but I found it a bit confusing, and I also think there can be people
like that too.

Renaming those variables might be a good idea too, but I couldn't find better
names. I find they get too long if I want to include their meanings in their
names. Do you have suggestions for the names?

Changes:

• Remove SizeTypeID, because it's more on the obvious side
• Delete 'usually' clauses

[zhmu]

This comment was removed by zhmu.

[zhmu]

While I thank you for your efforts, I really think this is a bad idea:

• If a comment is necessary to illustrate the point of a variable name, this means the variable name is wrong and we ought to change it
• Such comments tend to get outdated as no one reads/updates them anymore once the code is reworked

In this case, I'd say the variable names aren't that strange or need explanation in this degree. Perhaps the overall flow can be improved, but that is beyond the scope of simply adding a comment.

[dschuff]

I think it could be clearer than it is. The problem is that there are 2 overlapping naming conventions in use simultaneously:

• SizeFoo referring to the size of a foo object or objects, (SizeAction, SizeActions)
• BarAction referring to the index of a particular action (FirstAction, PrevAction).

Both of those are unsigned but mean very different things. So it's good that there's a convention but SizeAction fits into both and is ambiguous.
I think renaming SizeAction to SizeActionEntry would fix that. SizeActions is probably ok. I think a brief comment explaining the usage is fine but putting it inline with the declaration minimizes risk of it getting outdated.

So, how about the suggestions below:

lib/CodeGen/AsmPrinter/EHStreamer.cpp

105 ↗

(On Diff #168010)

unsigned SizeActions = 0; // Total size of all action entries for a function

113 ↗

(On Diff #168010)

unsigned SizeSiteActions = 0; // Total size of all entries for one lnadingpad

116 ↗

(On Diff #168010)

unsigned SizeActionEntry = 0; // Size of one action entry (typeid + next action)

[aheejin]

• Address comments

llvm/trunk/lib/CodeGen/AsmPrinter/EHStreamer.cpp

for (std::vector<unsigned>::const_iterator
          I = FilterIds.begin(), E = FilterIds.end(); I != E; ++I) {
     FilterOffsets.push_back(Offset);
     Offset -= getULEB128Size(*I);
   }
 
   FirstActions.reserve(LandingPads.size());
 
   int FirstAction = 0;
-  unsigned SizeActions = 0;
+  unsigned SizeActions = 0; // Total size of all action entries for a function
   const LandingPadInfo *PrevLPI = nullptr;
 
   for (SmallVectorImpl<const LandingPadInfo *>::const_iterator
          I = LandingPads.begin(), E = LandingPads.end(); I != E; ++I) {
     const LandingPadInfo *LPI = *I;
     const std::vector<int> &TypeIds = LPI->TypeIds;
     unsigned NumShared = PrevLPI ? sharedTypeIDs(LPI, PrevLPI) : 0;
-    unsigned SizeSiteActions = 0;
+    unsigned SizeSiteActions = 0; // Total size of all entries for a landingpad
 
     if (NumShared < TypeIds.size()) {
-      unsigned SizeAction = 0;
+      // Size of one action entry (typeid + next action)
+      unsigned SizeActionEntry = 0;
       unsigned PrevAction = (unsigned)-1;
 
       if (NumShared) {
         unsigned SizePrevIds = PrevLPI->TypeIds.size();
         assert(Actions.size());
         PrevAction = Actions.size() - 1;
-        SizeAction = getSLEB128Size(Actions[PrevAction].NextAction) +
+        SizeActionEntry = getSLEB128Size(Actions[PrevAction].NextAction) +
                           getSLEB128Size(Actions[PrevAction].ValueForTypeID);
 
         for (unsigned j = NumShared; j != SizePrevIds; ++j) {
           assert(PrevAction != (unsigned)-1 && "PrevAction is invalid!");
-          SizeAction -= getSLEB128Size(Actions[PrevAction].ValueForTypeID);
-          SizeAction += -Actions[PrevAction].NextAction;
+          SizeActionEntry -= getSLEB128Size(Actions[PrevAction].ValueForTypeID);
+          SizeActionEntry += -Actions[PrevAction].NextAction;
           PrevAction = Actions[PrevAction].Previous;
         }
       }
 
       // Compute the actions.
       for (unsigned J = NumShared, M = TypeIds.size(); J != M; ++J) {
         int TypeID = TypeIds[J];
         assert(-1 - TypeID < (int)FilterOffsets.size() && "Unknown filter id!");
         int ValueForTypeID =
             isFilterEHSelector(TypeID) ? FilterOffsets[-1 - TypeID] : TypeID;
         unsigned SizeTypeID = getSLEB128Size(ValueForTypeID);
 
-        int NextAction = SizeAction ? -(SizeAction + SizeTypeID) : 0;
-        SizeAction = SizeTypeID + getSLEB128Size(NextAction);
-        SizeSiteActions += SizeAction;
+        int NextAction = SizeActionEntry ? -(SizeActionEntry + SizeTypeID) : 0;
+        SizeActionEntry = SizeTypeID + getSLEB128Size(NextAction);
+        SizeSiteActions += SizeActionEntry;
 
         ActionEntry Action = { ValueForTypeID, NextAction, PrevAction };
         Actions.push_back(Action);
         PrevAction = Actions.size() - 1;
       }
 
       // Record the first action of the landing pad site.
-      FirstAction = SizeActions + SizeSiteActions - SizeAction + 1;
+      FirstAction = SizeActions + SizeSiteActions - SizeActionEntry + 1;
     } // else identical - re-use previous FirstAction
 
     // Information used when creating the call-site table. The action record
     // field of the call site record is the offset of the first associated
     // action record, relative to the start of the actions table. This value is
     // biased by 1 (1 indicating the start of the actions table), and 0
     // indicates that there are no actions.
     FirstActions.push_back(FirstAction);