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

107

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

115

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

118

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

[aheejin]

• Address comments

lib/CodeGen/AsmPrinter/EHStreamer.cpp

void EHStreamer::computeActionsTable(
   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());
 
+  // SizeActions: size of all action entries for a function
+  // SizeSiteActions: size of all action entries for one landing pad
+  // SizeAction: size of an action entry (usually 2, size of typeid + size of
+  //             next action)
+  // SizeTypeID: size of typeid (usually 1)
   int FirstAction = 0;
   unsigned SizeActions = 0;
   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;
 
     if (NumShared < TypeIds.size()) {
       unsigned SizeAction = 0;
       unsigned PrevAction = (unsigned)-1;
 
       if (NumShared) {
         unsigned SizePrevIds = PrevLPI->TypeIds.size();
         assert(Actions.size());
         PrevAction = Actions.size() - 1;
         SizeAction = getSLEB128Size(Actions[PrevAction].NextAction) +
                      getSLEB128Size(Actions[PrevAction].ValueForTypeID);