[PATCH] D89775: [lld][ELF][test] Add additional test coverage for LTO

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM.



================
Comment at: lld/test/ELF/lto/bitcode-wrapper.ll:27
+
+; ERR1: error: [[FILE]]: Invalid bitcode wrapper header
+
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > grimar wrote:
> > > > > aside: we probably want to lowercase this message.
> > > > I took a quick look, and this is just one of several error messages within the BitcodeReader and BitcodeAnalyzer files, and most of them use upper-case, so that would probably want to be part of a much wider change. Think we'd need to discuss the preferred style of the library with whoever owns/maintains it before doing that.
> > > > Think we'd need to discuss the preferred style of the library with whoever owns/maintains it before doing that.
> > > 
> > > perhaps we could just lower case all error messages on the lld side.
> > I thought about that, but that would risk us occasionally converting strings to lower-case that shouldn't be (e.g. file paths, names, accronyms like ELF or DWARF etc).
> I meant to lower case just the first character. Perhaps it is a low risk here. Though probably
> a more safe way is to convert particular error messages. I.e. if it comes from BitcodeReader/BitcodeAnalyzer we could do it in case if we can't just switch to lower case right there.
I'd prefer not transforming the error messages. If people search for "Invalid bitcode wrapper header", they can find occurrences in llvm/ and lld/test/ELF. If they happen to change the messages (e.g. to add more information; many BitcodeReader.cpp messages are duplicated in multiple places and are generally not so informational), they can update the lld one as well. Suredly they can use grep -i or rg -i, or not search for the first character, but that is extra hurdle.

We lose a bit of inconsistency, but the loss seems very small to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89775