This is an archive of the discontinued LLVM Phabricator instance.

[objcopy][NFC] Move NameOrPattern::create() into CommonConfig.h
ClosedPublic

Authored by avl on Mar 4 2022, 9:06 AM.

Details

Summary

While moving objcopy into separate library(D88827), NameOrPattern::create()
was mistakenly placed into ObjcopyOptions.cpp. This patch moves
the NameOrPattern::create() into CommonConfig.h. Additionally it adds
test for using NameOrPattern.

Diff Detail

Event Timeline

avl created this revision.Mar 4 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: abrachet. · View Herald Transcript
avl requested review of this revision.Mar 4 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 9:06 AM
jhenderson added inline comments.Mar 6 2022, 10:41 PM
llvm/include/llvm/ObjCopy/CommonConfig.h
113–115 ↗(On Diff #413035)

Given the complexity of this function, I think it should be in a .cpp file.

llvm/unittests/ObjCopy/ObjCopyTest.cpp
400–403

It seems to me like this function has a lot of copied code in it. I think you should be able to share most of it rather than just copying.

413
470

If this YAML is identical to other cases, pull it into a shared constant somewhere.

avl updated this revision to Diff 413431.Mar 7 2022, 4:44 AM

addressed comments(moved NameOrPattern::create() into .cpp, splitted functions into several smaller in ObjCopyTest.cpp)

avl added inline comments.Mar 7 2022, 4:48 AM
llvm/unittests/ObjCopy/ObjCopyTest.cpp
470

I suggest to not use shared constants here. These are different places(even if yaml descriptions look similar). Changing in one place should not necessarily be reflected in others. Also it is more convenient to see them inplace rather then moved to constant definition.

Though if you still think we need shared constants here - will update the patch accordingly.

jhenderson added inline comments.Mar 7 2022, 11:50 PM
llvm/lib/ObjCopy/CommonConfig.cpp
17

I'm not sure you need the llvm:: prefix.

llvm/unittests/ObjCopy/ObjCopyTest.cpp
142

Nit here and below: LLVM style says use loweer case for start of error messages.

152

Sec is a more common abbreviation for "section". Applies elsewhere too.

153
165
188

Maybe?

418

Do you need the trailing return type?

470

I think it should be shared: if at some point it needs to be unshared, people can make the required change then. Similarly, future developers should be careful not to make changes to shared code without understanding the implications.

As a direct counterpoint to your comment: what if e.g. the YAML format changed slightly, necessitating a change to the input strings: you wouldn't want to modify the same s tring multiple timesm would you?

avl added inline comments.Mar 8 2022, 12:59 AM
llvm/unittests/ObjCopy/ObjCopyTest.cpp
470

I think it should be shared: if at some point it needs to be unshared, people can make the required change then. Similarly, future developers should be careful not to make changes to shared code without understanding the implications.

Ok, would change to shared constants.

As a direct counterpoint to your comment: what if e.g. the YAML format changed slightly, necessitating a change to the input strings: you wouldn't want to modify the same s tring multiple timesm would you?

That would be mechanical search&replace. It looks not a problem from my point of view.

jhenderson added inline comments.Mar 8 2022, 1:02 AM
llvm/unittests/ObjCopy/ObjCopyTest.cpp
470

That would be mechanical search&replace. It looks not a problem from my point of view.

Assuming the developer knew there were multiple places to change and the nature of the change is straightforward enough to be amenable to that.

avl updated this revision to Diff 413746.Mar 8 2022, 2:36 AM

addressed comments.

jhenderson accepted this revision.Mar 8 2022, 11:13 PM

LGTM, with one suggestion.

llvm/unittests/ObjCopy/ObjCopyTest.cpp
24

Simpler name: SimpleFileCOFFYAML. Similar for the others.

This revision is now accepted and ready to land.Mar 8 2022, 11:13 PM
avl added a comment.Mar 9 2022, 2:03 AM

Ok. Thanks!

This revision was landed with ongoing or failed builds.Mar 9 2022, 2:04 AM
This revision was automatically updated to reflect the committed changes.