From: Olivier Certner <ocert.dev@free.fr>
To: Alan Mackenzie <acm@muc.de>
Cc: 63286@debbugs.gnu.org
Subject: bug#63286: 30.0.50; CC Mode: New `c-for-clauses-as-arglist' style variable
Date: Tue, 09 May 2023 12:08:55 +0200 [thread overview]
Message-ID: <1769415.c5zRbCDrAu@ravel> (raw)
In-Reply-To: <ZFkO69nS8ACiHgGN@ACM>
Hello Alan,
> > I guess you mean "radical" because this is an engine modification?
>
> Not really. It proposes introducing a new style variable, for one
> thing. But it also proposes making the syntactic expressions produced
> for some constructs non-constant, and that where there isn't a bug to
> solve.
It proposes a new style variable, in addition to the already existing 16 (!).
Syntactic contexts are constant as long as the variable keeps the same value,
but yes, they differ depending on the particular chosen value, and that's
exactly the point of this new setting. This is no different than how `c-
syntactic-indentation-in-macros' works. Finally, design and architecture
don't necessary need to be held up until there are actual bugs to solve.
> I don't find it elegant.
Elegance in this matter, at least in my book, is reaching objectives with the
minimal amounts of code at the right place(s) while preserving or enhancing
the design (that's perhaps a reductive definition, but it is enough for the
current discussion). I wouldn't have bothered submitting this patch to the
engine if it didn't enhance the current architecture. And it does so because
it allows a logical change in behavior with a single switch at the right place
rather than tweaking many small pieces after the fact to achieve it.
> The style variables are intended for widely used features in CC Mode.
I'm generally skeptical on most popularity contexts since they are usually
biased, either statistically or conceptually. But I'll contribute an
additional data point: I certainly did not have to use any of them in 20+
years until recently, except for `c-basic-offset' and `c-offsets-alist' of
course. If you're stating a policy, then I don't think it is written anywhere
in the doc or in code comments. And I think the current situation may just be
proof of its non-existence. If it's a new one, than I hope I can convince you
it shouldn't apply in this case.
> To introduce another one just for a particular indentation would disrupt
> the logical consistency of CC Mode.
Would "disrupt the logical consistency of CC Mode"? Nothing less? Could you
elaborate on what is the consistency that this change threatens exactly? I
can only hope that you won't be exactly repeating the points I already
responded to above, since they are unconvincing at best and currently appear
to be great exaggerations. On the other hand more explanations would be more
than welcome.
> There have been many fixes made to indentation over the years, and if
> even just a few of these involved adding new style variables, the style
> system would be more confusing and less usable that it is.
There are changes and changes. I would not be surprised that most don't call
for a new style variable. This is simply not the case here.
> It is also easy, particularly in the current case. CC Mode is designed
> to make it easy. You could do this with a single lineup function which
> would return either the desired offset, or nil when the function is
> inapplicable. The c-offsets-alist entries for the three (or four?)
> pertinent syntactic symbols would then be modified to lists beginning
> with the symbol `first' and followed by the new function then the
> default. The new function would likely be well under 20 lines (not
> counting comments). See the page "c-offsets-alist" in the CC Mode
> manual.
I know the manual in and out, and the code realizing the indentation very
well. I've also written a bunch of non-trivial lineup functions (comparable
to some bundled in `cc-align.el') and I'm already using the excellent
combining facilities provided by offset lists.
And with that knowledge, I have to disagree. Writing lineup functions is not
easy, it can be tedious and messy, even if in the end the code doesn't need to
be very long. Because of a certain lack of coherency in how syntactic
contexts are built (and lack of sufficient documentation for that process), it
is indeed easy to forget cases, which only appear later when indenting some
new or yet unexplored code. Sure, you can write lineups very quickly for toy
examples, but this is not what I'm aiming at (and you're now aware of the
larger context).
I don't claim to have years of experience with the internals of CC mode and
lineups, contrary to you. Yet, in addition to what I already exposed, I've
carefully analyzed a large part of CC Mode code in the past days (I've not
counted, but based on the number of files and their size, very roughly between
30 and 50% of the code base), that's also why I'm confidently making these
statements. I might be wrong. On the other hand, I'm also beginning to
wonder if your ~20-year maintainership of CC Mode is simply biasing you on the
matter.
Indeed, don't you see any problem with:
"The c-offsets-alist entries for the three (or four?) pertinent syntactic
symbols would then be modified to lists beginning with the symbol `first' and
followed by the new function then the default."
It doesn't have to be like that. And it is not going to take only a single
`first' list. And we're only talking about a single issue in the journey to
implement BSD KNF, there are a lot more requiring even more complexities in
offsets and new lineup functions. I know that since I have working code for
the whole thing.
> The treatment is already uniform - it just doesn't currently allow the
> indentation you want in your new style.
Here we are definitely leaving the realm of points of view or opinions for
that of plain facts. And unless we do not have the same words, this statement
is just completely wrong.
;; CASE 7D: we are inside a conditional test clause. treat
;; these things as statements
((progn
(goto-char containing-sexp)
(and (c-safe (c-forward-sexp -1) t)
(looking-at "\\<for\\>[^_]")))
(goto-char (1+ containing-sexp))
(c-forward-syntactic-ws indent-point)
(if (eq char-before-ip ?\;)
(c-add-syntax 'statement (point))
(c-add-syntax 'statement-cont (point))
))
See the `looking-at' call matching the `for' keyword? If this is not special
casing, I don't know what this is. Case 7D is a sub-case of case 7, whose aim
is to match expressions, not statements. Following subcases (7F to 7G)
actually parse parenthesized expressions (I'm omitting 7E which concerns ObjC
only).
So, this case specifically matches a `for' on the path to parsing
parenthesized expressions, and in a branch which is not supposed to parse
statements. QED.
> No, there have been no other bug reports about this in the last 20 years
> or so, at least. Yours is the first one.
Then at least there won't be things repeated. But this gives me even more
responsibility, speaking up for all those that didn't voice their concern.
Which, now that I vaguely recall, probably included me over 20 years ago in a
very different context.
And, again, I know of no C editor behaving the same as Emacs+CC Mode on `for'
clauses, and certainly not the popular ones (but I also do not know all of
them, and have not used some for years; please someone point out to those that
would do).
> The treatment of `for' in CC Mode is not a special case in CC Mode.
Sorry, but it simply is. See above (the part commenting case 7D) and just
below (next paragraph).
> If anything, it's a special case in the language definitions, where
> executable code is enclosed in parentheses.
This is also plain wrong. There's no more "executable code" (this sentence is
not used anywhere in the standard) in `for' clauses than in expressions
(parenthesized or not). In fact, all but the first clause are just
expressions in the standard, no more. Only the first can be a declaration
instead. None of the clauses are statements. You're mixing things up. I
encourage you to read the standard so you don't have to take my word for it.
> I think there are rather _several_ different situations rather than many.
You're playing on words but I think you've got the idea. The main point is
that this complexity is unnecessary. Still, you're underestimating the number
of corner cases.
> I don't foresee the troubles that you do. By the time the lineup
> function comes to be run, things like extra parenthesised expressions
> have already been analysed by the indentation engine. The anchor
> positions allow one readily to find the opening parenthesis and the
> preceding `for' token, or determine that they don't exist in the current
> case.
Not only I foresee them, but I've already experienced some of them.
And I already know what you're saying. It would be perfect if that was all
there was to it. Unfortunately, it's not the case. You're omitting for
example the "special case" of having nested parenthesized groups with opening
parentheses all on the same line.
> I think a single 20 line function could do the trick, as outlined above.
> The extra run time (if any) taken would be too small to measure.
May be, I'm not sure. CC Mode is already very slow for full file indentation
(I know it's not its goal, but still). But whether that matters or not, what
is certain is that implementing a special case in an engine to subsequently
undo it in a hook is simply bloat and a waste of cycles, however small it is.
> That it would fragment CC Mode, reducing its cohesiveness and
> introducing special cases where none are needed. It would entail
> changes to the test suite, which would complicate it, and substantial
> changes to the documentation.
I couldn't disagree more.
What you are proposing is a lineup function that is itself essentially a large
`cond' of special cases just to undo an engine's very localized special case.
The proposed approach instead just selectively deactivates the latter in the
first place.
Adding only a few new tests is more than enough, because there is provably no
change at all if the variable is not explicitly set and because changes are
confined to `for' clauses only. How does this "complicate" the suite? If
adding a few tests is a problem, then the test suite is the problem.
Addendum: I cannot find anything in the source tree but a mere 'cc-mode-
tests.el' that is 111 lines long (including comments). Where is the "suite"
you're talking about?
Even if you want to be comprehensive, on this precise change there's not much
more to document than what is already in the patch. Of course, a lot should
be added to explain all the special cases of the engine, but this is
independent from this report. Again, you're talking about "substantial
changes in the documentation", but you don't mention what you have in mind.
Finally, as for "fragmentation" or threat to "cohesiveness", I'm sorry to have
to say that I have yet to read a single precise and relevant argument that can
really back this point of view.
> In short, I don't think the approach you've presented is the right one
> to get the indentation that you need.
Then I'm asking you to step back and then reconsider, because you're most
likely wrong. At the very least, filling the gaps between the vague high-
level principles you stated (and which I generally support) and how the
proposed change actually breaks them, as well as answering my other questions,
would be very welcome. If you're not willing to, then I'm not sure there is
any point in continuing this discussion.
Regards.
--
Olivier Certner
next prev parent reply other threads:[~2023-05-09 10:08 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-04 22:19 bug#63286: 30.0.50; CC Mode: New `c-for-clauses-as-arglist' style variable Olivier Certner
2023-05-04 22:22 ` Olivier Certner
2023-05-05 4:57 ` Eli Zaretskii
2023-05-06 21:49 ` Olivier Certner
2023-05-05 11:14 ` Alan Mackenzie
2023-05-06 21:41 ` Olivier Certner
2023-05-08 15:02 ` Alan Mackenzie
2023-05-09 10:08 ` Olivier Certner [this message]
2023-05-09 15:11 ` Alan Mackenzie
2023-05-10 13:20 ` Olivier Certner
2023-05-10 14:26 ` Eli Zaretskii
2023-09-11 18:14 ` Stefan Kangas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1769415.c5zRbCDrAu@ravel \
--to=ocert.dev@free.fr \
--cc=63286@debbugs.gnu.org \
--cc=acm@muc.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.