[PATCH] D82001: [llvm] [CommandLine] Do not suggest really hidden opts in nearest lookup

[Nathan James via Phabricator via llvm-commits]

njames93 added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:595
     Option *O = it->second;
+    // Do not suggest really hidden options (not shown in any help).
+    if (O->getOptionHiddenFlag() == ReallyHidden)
----------------
mgorny wrote:
> sammccall wrote:
> > Rather than not considering them (and offering a worse option, which may be confusing) you might consider checking `Best` after the loop so we just offer no suggestion if the best one is hidden.
> > 
> > Not sure which behavior is better, up to you.
> The clang-tools-extra tests in question rely on having the 'next best' served.
> 
> I think the current behavior is more predictable, as adding new 'really hidden' options won't cause the current suggestions to suddenly disappear.
Are they the ones added in D80879. If it is, the CHECK-NEXT lines in there could probably be done away with anyway. The important part was making sure it didn't crash.

However tests in `llvm/unittests/Support/CommandLineTests` should be added for this change.


================
Comment at: llvm/lib/Support/CommandLine.cpp:595
     Option *O = it->second;
+    // Do not suggest really hidden options (not shown in any help).
+    if (O->getOptionHiddenFlag() == ReallyHidden)
----------------
njames93 wrote:
> mgorny wrote:
> > sammccall wrote:
> > > Rather than not considering them (and offering a worse option, which may be confusing) you might consider checking `Best` after the loop so we just offer no suggestion if the best one is hidden.
> > > 
> > > Not sure which behavior is better, up to you.
> > The clang-tools-extra tests in question rely on having the 'next best' served.
> > 
> > I think the current behavior is more predictable, as adding new 'really hidden' options won't cause the current suggestions to suddenly disappear.
> Are they the ones added in D80879. If it is, the CHECK-NEXT lines in there could probably be done away with anyway. The important part was making sure it didn't crash.
> 
> However tests in `llvm/unittests/Support/CommandLineTests` should be added for this change.
I'm not too fond of Sams suggestion as it isn't great for the case when a really hidden option had a slightly shorter distanced name to another non hidden option, You will lose the valid option suggestion. 
I kind of prefer the way of ignoring really hidden options, but putting an upper limit on the distance for suggested changes. I almost feel like the distance should be a function of the OptionLength - A 50 character option differing by 2 characters has much more confidence than a 3 character option differing by 2. Can't think of a good mapping for it though, My first suggestion would be based on the square root of the length


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

https://reviews.llvm.org/D82001