[PATCH] D62072: Worked on bug 40859 to be more consistent with error messages in llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the patch @abrachet! I have some comments that you may find useful both here and in the future, regarding the title/description:

In general, title the issue the same as you would the actual commit summary. It's also often useful to put a tag in front representing the tool/library that is being modified (this helps people with searches/email filters etc). Also, put in the description what would go in the commit body (excluding extra bits like the "Differential Revision" and reviewers list. Also, bugs tend to be referred to as PR123456 or simply by their URL in patches. Here's what I'd recommend:

Title: [llvm-objcopy]Tidy up error messages
Description: This patch brings various error messages into line with each other, by removing trailing full stops, and making the first letter lower-case. This addresses https://bugs.llvm.org/show_bug.cgi?id=40859.

It's also useful to put the link to the review request in the bug comments, so that anybody watching the bug can review the patch.

I'm expecting to see a number of test changes. Please make sure to run the lit tests contained within llvm/test/tools/llvm-objcopy with your changes built. This will likely show a number of test failures where we check the error message string, and you'll need to update those too.



================
Comment at: llvm/tools/llvm-objcopy/COFF/Object.cpp:56
         return createStringError(object_error::invalid_symbol_index,
-                                 "Relocation target %zu not found", R.Target);
+                                 "relocation target '%zu' not found", R.Target);
       It->second->Referenced = true;
----------------
I'm not sure this needs to be quoted, as it is a number, rather than a string.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Reader.cpp:186
         return createStringError(object_error::parse_failed,
-                                 "SymbolTableIndex out of range");
+                                 "symbolTableIndex out of range");
       const Symbol *Sym = RawSymbolTable[R.Reloc.SymbolTableIndex];
----------------
I think SymbolTableIndex is a variable name, based on the COFF file format, and so should match the actual name. It might be worth confirming this, and if it is, you shouldn't change the case here. If it isn't a part of the file format spec, then it should be something like "symbol table index".


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:171
   return createStringError(llvm::errc::invalid_argument,
-                           "Could not find build ID.");
+                           "could not find build ID.");
 }
----------------
Remove the full stop here.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:288
   }
-  return createStringError(object_error::parse_failed, "Section not found");
+  return createStringError(object_error::parse_failed, "section '%s' not found", SecName.str().c_str());
 }
----------------
You need to run clang-format here to format this to the LLVM style.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1260-1263
+            "e_shstrndx field value '" + Twine(Ehdr.e_shstrndx) +
+                "' in elf header is invalid",
+            "e_shstrndx field value '" + Twine(Ehdr.e_shstrndx) +
+                "' in elf header is not a string table");
----------------
I don't think you need to quote numeric values. The main reason for quoting section/symbol names is because they theoretically can contain spaces.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1629
+                             "cannot write section header table because "
                              "section header string table was removed.");
 
----------------
There's still a trailing full stop here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62072