[PATCH] D66015: [llvm-strings] Improve testing of llvm-strings

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-strings/all-sections.test:1
-RUN: llvm-strings -a %S/Inputs/abcd | FileCheck %s
-RUN: llvm-strings --all %S/Inputs/abcd | FileCheck %s
+# Show that the -a/--all switch is accepted. GNU strings checks specific object
+# file sections unless --all is specified. llvm-strings just looks at the entire
----------------
jhenderson wrote:
> grimar wrote:
> > Should we use `##` in this tests too? I would do this, since it is probably
> > not bad to have a single style across the all LLVM tests.
> I didn't bother here because this isn't used as an input to anything, but I can do so if you're bothered. Should I add '#' to the RUN and CHECK lines too? I'd prefer not to, mostly because that will unnecessarily make git blame less useful.
I'd use '##' fo comments, I agree it is probably no need to use them for `RUN`/`CHECK` lines.


================
Comment at: test/tools/llvm-strings/eof.test:5
 RUN: echo -n abcdefg | llvm-strings - | FileCheck %s
 CHECK: abcdefg
----------------
jhenderson wrote:
> grimar wrote:
> > What about combining this and `eof-no-string.test` test into one?
> I could do that, but I kept them separate because that's how they were previously. If you'd like me to fold them together, I can do so.
Phab did not show the renaming, so I assumed this is the new file. 
Since it is not, it is OK to leave it as is in this patch I think.

(I would fold them in a followup though).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66015