Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

I prefer east/west, but I think there's a terminology that is
uncontroversial (left/right) and one that is controversial. It's also clear
to me that it's better to have only one set of terms (aliases are only fine
for backwards compatibility). Left/Right won, I think.

On Tue, May 26, 2020 at 1:55 PM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> sammccall added a comment.
>
> Thanks @MyDeveloperDay for hammering on on these bugs and @steveire for
> finding them! There's still obviously some risk here but as long as this is
> opt-in for a major release or two (i.e. not turned on in built-in styles,
> any remaining bugs should get flushed out.
>
> Regarding option naming, I did think East/West (only) was the way to go
> but have reluctantly changed my mind after rereading the discussion. My
> conclusions were:
>
> - C++ people who have read discussions on this from 2018 on are likely
> familiar with "east const" terminology
> - there are people who care about style who aren't familiar with it and
> wouldn't be happy to have to learn/adopt it (this was the main surprise for
> me)
> - 5 years from now, this naming may have spread to ~everyone, may remain
> partially-adopted, or possibly even die out as a fad
> - for people familiar with the terminology: "ConstStyle: East" is clearer
> than "ConstStyle: Right" (less ambiguous)
> - for people unfamiliar with the terminology, the opposite is certainly
> true
> - asymmetry 1: it's probably harder to work out east=right than to to work
> out that "ConstStyle: right" means const is written on the right of the type
> - asymmetry 2: the new terminology is objectively better: terse,
> memorable, doesn't collide with other terms. Some dislike it, which is true
> of any distinctive name (I hate "namespace", for instance).
> - there's clearly a cultural tension between reading like a meme-infested
> subreddit and like an IBM technical manual :-)
>
> It's a tough call, but I'd propose a compromise: make Left/Right canonical
> as it's more accessible (don't have to learn new things) and in case
> East/West dies out.
> But to have East/West as aliases: to let the community decide over time,
> and because I don't think we should be in the business of hindering
> adoption of better names.
>
> ("reluctantly" changed my mind because I do think east/west are better
> names. But meeting users where they are is important too - otherwise we'd
> just hardcode the One True Style :-))
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

It's very difficult to use a compile_commands database if you can't
actually check out all the code and a remote service builds it for you.

On Tue, Jul 13, 2021 at 6:03 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added a comment.
>
> In D69764#2874404 ,
> @MyDeveloperDay wrote:
>
> >> What you're describing sounds like clang-tidy but perhaps with improved
> support for composing source modifications from fix-its, or do you envision
> something rather different?
> >
> > All my uses of clang-tidy require extensive command line arguments to
> handle compiler flags and include paths, I don't want that for this when
> its mostly unnecessary:
> >
> >file.cxx
> >
> > is all that should be required.
>
> FWIW, if you use the compile commands database, the only thing you need to
> do on the command line is specify the checks to enable or disable.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

+1 for not only handling "const". I've often tried getting the various bits
that appertain to a declaration (static const volatile constexpr inline
consteval) sorted in a consistent order - that makes them much more
greppable.

Different patch, I expect, though.

On Wed, Jul 14, 2021 at 1:47 PM Marek Kurdej via Phabricator <
revi...@reviews.llvm.org> wrote:

> curdeius added a comment.
>
> I've been trying to make my opinion on this patch for the last few weeks...
> I was pretty much opposed to introducing non-whitespace chances
> previously, but I'm reviewing my standpoint.
> As mentioned already, there are precedents (include sorting, namespace
> comments, long string splitting).
> I'm however even more wary of adding yet another tool that will be almost
> the same as clang-format. It could work if it were a drop-in replacement of
> clang-format, but that seems to be very much wishful thinking to me.
> First, maintenance burden to verify that the two don't diverge somehow.
> Secondly, the drop-in replacement wouldn't be possible without changing
> clang-format itself (e.g. to ignore style options that are for
> "clang-format++" only). Also, it might divide the users into clang-format
> camp and clang-format++ camp (which may or may not be a problem).
> Lastly, I do think that clang-format can be as reliable with this patch as
> it's now. Breaking code is of course possible but that's the case of many
> style options. And these are bugs that will eventually get fixed. It's of
> course important that this option doesn't break anything ever by default,
> but given that the default is Leave, and it's implemented as an additional
> pass, that should be the case.
> Also, I'd be a bit surprised if people used it in CI immediately after
> this feature has landed without verifying that it doesn't break anything on
> their codebase.
>
> On the other hand, clang-tidy has a corresponding check. I do feel though
> that's a sort of heavyweight tool and much less used than clang-format.
> Also, the placing of const qualifier is by many (at least in my circles)
> associated to the latter tool.
>
> So yes, I'm in favour of landing this patch (though not exactly in the
> current form, I'd prefer more future-proof options for instance, not only
> handling const).
> My (longish) 2 cents.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D69764: [clang-format] Add East/West Const fixer capability

[Gašper Ažman via cfe-commits]

It would probably be better to make the config option names for passes that
may mutate whitespace be prefixed with MaybeIncorrect than fork the tool.
Scary options are better than forks.

On Wed, Jul 14, 2021 at 6:42 PM MyDeveloperDay via Phabricator <
revi...@reviews.llvm.org> wrote:

> MyDeveloperDay added a comment.
>
> I wanted to address your other points so we at least can be aligned as I
> respect your opinion.
>
> > I'm however even more wary of adding yet another tool that will be
> almost the same as clang-format.
>
> Also agreed, but I see no other way forward.
>
> > It could work if it were a drop-in replacement of clang-format, but that
> seems to be very much wishful thinking to me.
>
> This is exactly my suggestion. D105943: [clang-format++] Create a new
> variant of the clang-format tool to allow additional  code mutating
> behaviour such as East/West Const Fixer 
> will be a new clang-format for those that accept the fact it will modify,
> why those who can't just can't NOT turn the options on  in the first place,
> I'm not quite sure, but I'm trying to be inclusive of everyone and find a
> road to a solution which is good for those that want it and those that don't
>
> If we create a new tool, I recommend you, I and some of the other
> clang-format regulars also be the CODE_OWNERS so we can innovate without
> feeling stifled.
>
> > First, maintenance burden to verify that the two don't diverge somehow.
>
> My aim would be to move the grunt of the  ClangFormat.cpp into lib/Format,
> so both processes could share them, the main() would effective become
> calling the same submain() but with a "Yes please mutate all you like
> variable"
>
> > Secondly, the drop-in replacement wouldn't be possible without changing
> clang-format itself (e.g. to ignore style options that are for
> "clang-format++" only).
>
> No I wouldn't do this, the old clang-format would ignore mutating passes
> the new one wouldn't, the style options would remain for both so they can
> both share a common .clang-format file (lets remember I don't want to do it
> this way!)
>
> > Also, it might divide the users into clang-format camp and
> clang-format++ camp (which may or may not be a problem).
>
> Of course, and it would "Fragment the Community", but this is what happens
> when some want to innovate and others like it the way it is (we see this
> all the time from mailing lists to bug tracker), but it doesn't have to be
> this way, we could simply have one binary to rule them all and keep in the
> community that you and I spend a significant amount of our time giving back
> to.
>
> > Lastly, I do think that clang-format can be as reliable with this patch
> as it's now.
>
> Me too.
>
> > Also, I'd be a bit surprised if people used it in CI immediately after
> this feature has landed without verifying that it doesn't break anything on
> their codebase.
>
> No I use CI in an advisory capacity with the -n or (--dry-run) option just
> to highlight violations not change them)
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D69764/new/
>
> https://reviews.llvm.org/D69764
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits