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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:439
+      R"(
+--- !COFF
+header:
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > jhenderson wrote:
> > > > If this YAML is identical to other cases, pull it into a shared constant somewhere.
> > > 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.
> > 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?
> >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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121005/new/

https://reviews.llvm.org/D121005