This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sbc100 on Jan 11 2018, 4:13 PM.

Details

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.

Event Timeline

sbc100 created this revision.Jan 11 2018, 4:13 PM
sbc100 edited the summary of this revision. (Show Details)Jan 11 2018, 4:15 PM
sbc100 added a reviewer: vsk.
sbc100 edited the summary of this revision. (Show Details)
vsk added a comment.Jan 11 2018, 4:51 PM

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?

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.

sbc100 retitled this revision from MC: Remove redundant `SetUsed` arguments in MCSymbol methods to MC: Remove redundant `SetUsed` arguments in MCSymbol methods. NFC.Jan 11 2018, 4:57 PM
vsk added a comment.Jan 11 2018, 5:02 PM
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.

I can take a look.

espindola accepted this revision.Jan 12 2018, 7:59 AM
espindola added a subscriber: espindola.

LGTM.

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

This revision is now accepted and ready to land.Jan 12 2018, 7:59 AM
This revision was automatically updated to reflect the committed changes.