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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjCopy/CommonConfig.cpp:16
+NameOrPattern::create(StringRef Pattern, MatchStyle MS,
+                      llvm::function_ref<Error(Error)> ErrorCallback) {
+  switch (MS) {
----------------
I'm not sure you need the `llvm::` prefix.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:64
+  if (!IsValidFormat(**Result))
+    return createError("Wrong file format");
+
----------------
Nit here and below: LLVM style says use loweer case for start of error messages.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:74
+bool hasSection(ObjectFile &File, StringRef SectionName) {
+  for (const object::SectionRef &Sect : File.sections()) {
+    Expected<StringRef> CurSectNameOrErr = Sect.getName();
----------------
`Sec` is a more common abbreviation for "section". Applies elsewhere too.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:75
+  for (const object::SectionRef &Sect : File.sections()) {
+    Expected<StringRef> CurSectNameOrErr = Sect.getName();
+    if (!CurSectNameOrErr)
----------------



================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:87
+// Check that specified \p File has a section \p SectionName and its data
+// match with \p SectionData.
+void checkSectionData(ObjectFile &File, StringRef SectionName,
----------------



================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:110
+    const char *YamlCreationString,
+    std::function<bool(const Binary &File)> IsValidFormat) {
+  SCOPED_TRACE("copySimpleInMemoryFileImpl");
----------------
Maybe?


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:439
+      SectionWildcard, objcopy::MatchStyle::Wildcard,
+      [](Error Err) -> Error { return Err; });
+
----------------
Do you need the trailing return type?


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:439
+      R"(
+--- !COFF
+header:
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121005