This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by aheejin on Sep 28 2018, 6:28 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Sep 28 2018, 6:28 PM
kristina added a subscriber: 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.

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?

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.

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 updated this revision to Diff 168010.Oct 2 2018, 1:28 PM

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 added a subscriber: zhmu.Oct 3 2018, 2:38 AM
This comment was removed by zhmu.
zhmu added a comment.Oct 3 2018, 2:41 AM

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.

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 updated this revision to Diff 168148.Oct 3 2018, 11:44 AM
  • Address comments
aheejin retitled this revision from Add comments explaning variables in action table generation (NFC) to Make meanings of variables clearer in action table generation (NFC).Oct 3 2018, 11:45 AM
aheejin edited the summary of this revision. (Show Details)
aheejin marked 3 inline comments as done.
dschuff accepted this revision.Oct 3 2018, 1:04 PM
This revision is now accepted and ready to land.Oct 3 2018, 1:04 PM
rnk accepted this revision.Oct 3 2018, 1:09 PM
This revision was automatically updated to reflect the committed changes.