[PATCH] D52857: Deprecate 'set output foo' API of clang-query

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D52857#1259522, @steveire wrote:

> > What's more, given that clang-output has no real documentation to speak of, how will users even *know* to update their scripts?
>
> The release notes will tell them.


That's not user friendly.

> But your comment also raises a different point: If there is so little clang-query documentation, what justification is there for relying on current behavior?

That's pretty much the only reason I think it's reasonable to consider deprecating the command in the first place. The justification for relying on it is: this has been the behavior since Day 1 of clang-query and we do not often remove user-facing options without there being a Very Good Reason to do so. This is not a situation where it's an undocumented feature that's in-flux or there's no indication it's being used.

> Also, what scripts are you referring to that rely on clang-query? Do we know of any that are expected to work long-term without maintenance (unlikely, as the AST itself is not stable), or is this concern imagined?

I'm referring to third-party scripts kept by people out of tree. I have several, I know a few other folks internally at my company who do as well.

I think it's reasonable to expect differences in AST output when updating clang-query, but this is not that. It's reasonable to expect command stability.

>> can you expound on your concerns?
> 
> After this patch, the user writes
> 
>   set dump-output true
> 
> 
> or `false`.
> 
> and after the follow-up (https://reviews.llvm.org/D52859) they write
> 
>   set matcher-output true
> 
> 
> After more features I have in the pipeline they will write other similar commands.
> 
> With my command-design, if a user wants to enable dump output in addition to what they have, they just
> 
>   set dump-output true
> 
> 
> In your design, they have to recall/find out what options are currently enabled, and add them in a comma separated list. That is not user-friendly.

Yes -- you are required to make a decision that you want to opt in to new functionality. The exact same thing is true with your proposal -- you have to find the right place to stick the `set dump-output true` in order to enable it.

>> wonder whether set blah-output options are mutually exclusive or not.
> 
> These options are obviously and clearly toggles.

Your notions of obvious and clear do not match mine.

> There is definitely no reason to think that they are mutually exclusive any more than there is reason to think `set output dump` is mutually exclusive with `set bind-root false`.

Well, except for the fact that each of these commands says "output" in the name and they are replacing a command argument named "output" that was mutually exclusive and there's zero documentation explaining the commands.

> And even if someone did wonder that, they would just try it and learn that they are toggles.

I don't see this functionality being so critical that we need to deprecate the existing spelling when there are backwards compatible options available, which is why I'm opposed to this patch going in with the proposed syntax.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52857