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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-strings/help.test:12
+CATEG: General options:
+CATEG: Generic Options:
 CHECK: @FILE
----------------
jhenderson wrote:
> grimar wrote:
> > Note: I am not sure why "Options" is in upper case when "Generic", but lower case when "General",
> > but my concern is that it makes the test case to be potentially brittle probably:
> > 
> > If you have `--implicit-check-not="Generic Options:"`, then if somebody deside to change "Option"->"option",
> > then you'll get a broken test case. Not sure if we should think about this scenario in that way though.
> I agree that the inconsistent case is bad. I'm not sure of a good alternative (apart from making it a regex pattern of [Oo]). I'll give it a bit more thought.
I've tried making it less likely to get missed, by putting the -NOT checks out-of-line, so that they're next to the positive checks. If the case is changed, hopefully the person updating the test will change both positive and negative cases.


================
Comment at: test/tools/llvm-strings/length.test:13
+RUN: llvm-strings -n 4 %t | FileCheck --check-prefix CHECK-4 %s --implicit-check-not={{.}}
+RUN: llvm-strings -n 5 %t | FileCheck --check-prefix CHECK-5 %s --implicit-check-not={{.}}
+
----------------
jhenderson wrote:
> grimar wrote:
> > What if `-n 6` or `-n -1` or `-n foo`?
> `-n -1` is actually broken. It looks like the command-line parser converts it to an unsigned, resulting in it being the equivalent of UINT_MAX. As GNU strings allows `-1` as an alias for `-n 1`, we probably don't want to try to address this immediately, and fix that issue first, then see if the problem is still possible afterwards.
> 
> I'll add `-n foo` and `-n 6` cases.
> -n -1 is actually broken. It looks like the command-line parser converts it to an unsigned

Turns out it's an int in the parser, so the parser accepts it, but it is then static_cast to a size_t! Anyway, still broken, and I've filed a bug for negative arguments.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D66015