GNU bug report logs - #23901
CC Mode 5.33 (Java/l); Wrong indentation of fallthrough switch/case with strings

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23901 in the body.
You can then email your comments to 23901 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Åsmund Grammeltvedt <asmundg <at> big-oil.org>
To: submit <at> debbugs.gnu.org
Subject: CC Mode 5.33 (Java/l); Wrong indentation of fallthrough switch/case
 with strings
Date: Tue, 5 Jul 2016 21:37:15 +0200

Hi!

Running indent-region on the following code produces wrong indentation
on all but the first statement in the last switch block.

I have included two cases that work correctly for comparison:

class Foo {
    public void Bar(String foo) {
        int a = 0;

        switch (a) {
        case 0:
        case 1:
            a += 1;
            a += 2;
            break;
        }

        switch (foo) {
        case "foo":
            a += 1;
            a += 2;
            break;
        }

        switch (foo) {
        case "foo":
        case "bar":
            a += 1;
        a += 2;
        break;
        }
    }
}


Emacs  : GNU Emacs 25.1.50.2 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8)
 of 2016-04-15
Package: CC Mode 5.33 (Java/l)
Buffer Style: java
c-emacs-features: (pps-extended-state col-0-paren posix-char-classes 
gen-string-delim gen-comment-delim syntax-properties 1-bit)

current state:
==============
(setq
 c-basic-offset 4
 c-comment-only-line-offset '(0 . 0)
 c-indent-comment-alist '((anchored-comment column . 0) (end-block 
space . 1) (cpp-end-block space . 2))
 c-indent-comments-syntactically-p nil
 c-block-comment-prefix "* "
 c-comment-prefix-regexp '((pike-mode . "//+!?\\|\\**") (awk-mode . 
"#+") (other . "//+\\|\\**"))
 c-doc-comment-style '((java-mode . javadoc) (pike-mode . autodoc) 
(c-mode . gtkdoc))
 c-cleanup-list '(scope-operator)
 c-hanging-braces-alist '((brace-list-open) (brace-entry-open) 
(statement-cont) (substatement-open after)
                          (block-close . c-snug-do-while) 
(extern-lang-open after) (namespace-open after)
                          (module-open after) (composition-open after) 
(inexpr-class-open after)
                          (inexpr-class-close before) 
(arglist-cont-nonempty))
 c-hanging-colons-alist nil
 c-hanging-semi&comma-criteria '(c-semi&comma-inside-parenlist)
 c-backslash-column 48
 c-backslash-max-column 72
 c-special-indent-hook nil
 c-label-minimum-indentation 1
 c-offsets-alist '((inexpr-class . +)
                   (inexpr-statement . +)
                   (lambda-intro-cont . +)
                   (inlambda . c-lineup-inexpr-block)
                   (template-args-cont c-lineup-template-args +)
                   (incomposition . +)
                   (inmodule . +)
                   (innamespace . +)
                   (inextern-lang . +)
                   (composition-close . 0)
                   (module-close . 0)
                   (namespace-close . 0)
                   (extern-lang-close . 0)
                   (composition-open . 0)
                   (module-open . 0)
                   (namespace-open . 0)
                   (extern-lang-open . 0)
                   (objc-method-call-cont 
c-lineup-ObjC-method-call-colons c-lineup-ObjC-method-call +)
                   (objc-method-args-cont . c-lineup-ObjC-method-args)
                   (objc-method-intro . [0])
                   (friend . 0)
                   (cpp-define-intro c-lineup-cpp-define +)
                   (cpp-macro-cont . +)
                   (cpp-macro . [0])
                   (inclass . +)
                   (stream-op . c-lineup-streamop)
                   (arglist-cont-nonempty c-lineup-gcc-asm-reg 
c-lineup-arglist)
                   (arglist-cont c-lineup-gcc-asm-reg 0)
                   (comment-intro c-lineup-knr-region-comment 
c-lineup-comment)
                   (catch-clause . 0)
                   (else-clause . 0)
                   (do-while-closure . 0)
                   (case-label . 0)
                   (substatement . +)
                   (statement-case-intro . +)
                   (statement . 0)
                   (brace-entry-open . 0)
                   (brace-list-entry . 0)
                   (brace-list-intro . +)
                   (brace-list-close . 0)
                   (brace-list-open . 0)
                   (block-close . 0)
                   (block-open . 0)
                   (inher-intro . +)
                   (member-init-cont . c-lineup-multi-inher)
                   (member-init-intro . +)
                   (annotation-var-cont . +)
                   (annotation-top-cont . 0)
                   (topmost-intro . 0)
                   (knr-argdecl . 0)
                   (inline-close . 0)
                   (class-close . 0)
                   (class-open . 0)
                   (defun-block-intro . +)
                   (defun-close . 0)
                   (defun-open . 0)
                   (c . c-lineup-C-comments)
                   (string . c-lineup-dont-change)
                   (func-decl-cont . c-lineup-java-throws)
                   (inher-cont . c-lineup-java-inher)
                   (access-label . 0)
                   (arglist-close . c-lineup-arglist)
                   (arglist-intro . c-lineup-arglist-intro-after-paren)
                   (statement-cont . +)
                   (statement-case-open . +)
                   (label . +)
                   (substatement-label . +)
                   (substatement-open . +)
                   (knr-argdecl-intro . 5)
                   (statement-block-intro . +)
                   (topmost-intro-cont . +)
                   (inline-open . 0)
                   )
 c-buffer-is-cc-mode 'java-mode
 c-tab-always-indent t
 c-syntactic-indentation t
 c-syntactic-indentation-in-macros t
 c-ignore-auto-fill '(string cpp code)
 c-auto-align-backslashes t
 c-backspace-function 'backward-delete-char-untabify
 c-delete-function 'delete-char
 c-electric-pound-behavior nil
 c-default-style '((java-mode . "java") (awk-mode . "awk") (other . "gnu"))
 c-enable-xemacs-performance-kludge-p nil
 c-old-style-variable-behavior nil
 defun-prompt-regexp nil
 tab-width 4
 comment-column 32
 parse-sexp-ignore-comments t
 parse-sexp-lookup-properties t
 auto-fill-function nil
 comment-multi-line t
 comment-start-skip "\\(//+\\|/\\*+\\)\\s *"
 fill-prefix nil
 fill-column 70
 paragraph-start "[ 	]*\\(//+\\|\\**\\)[ 	]*\\(@[a-zA-Z]+\\>\\|$\\)\\|^\f"
 adaptive-fill-mode t
 adaptive-fill-regexp "[ 	]*\\(//+\\|\\**\\)[ 	]*\\([ 
]*\\([-–!|#%;>*·•‣⁃◦]+[ 	]*\\)*\\)"
 )

-- 
Åsmund Grammeltvedt

Message #10 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Jostein Kjønigsen <jostein <at> secure.kjonigsen.net>
To: 23901 <at> debbugs.gnu.org
Cc: Alan Mackenzie <acm <at> muc.de>,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Subject: RE: #23901 CC Mode 5.33 (Java/l);
 Wrong indentation of fallthrough switch/case with strings
Date: Wed, 06 Jul 2016 10:49:34 +0200

Just thought I'd chip in and say that this bug has also been observed
in other third-party cc-mode derivatives, like csharp-mode.

As such it may affect quite a few c-like languages which has
switch/case-statements.

--
Jostein Kjønigsen
jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

Message #13 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Cc: 23901 <at> debbugs.gnu.org,
 Jostein Kjønigsen <jostein <at> secure.kjonigsen.net>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l); Wrong indentation of
 fallthrough switch/case with strings
Date: Wed, 6 Jul 2016 11:55:40 +0000

Hello, Åsmund.

On Tue, Jul 05, 2016 at 09:37:15PM +0200, Åsmund Grammeltvedt wrote:
> Hi!

> Running indent-region on the following code produces wrong indentation
> on all but the first statement in the last switch block.

OK.

> I have included two cases that work correctly for comparison:

Thank you for such a clear description of the fault, and thanks also for
including a CC Mode configuration dump (from C-c C-b).

> class Foo {
>      public void Bar(String foo) {
>          int a = 0;

>          switch (a) {
>          case 0:
>          case 1:
>              a += 1;
>              a += 2;
>              break;
>          }

>          switch (foo) {
>          case "foo":
>              a += 1;
>              a += 2;
>              break;
>          }

>          switch (foo) {
>          case "foo":
>          case "bar":
>              a += 1;
>          a += 2;
>          break;
>          }
>      }
> }

The problem appears to be in the CC Mode variable
`c-nonlabel-token-key', which specifies things which can't be in a
label.  The Java value includes the character '"', presumably from a
period in ancient history when strings in Java couldn't be case labels.

Please try out the following fix, and please confirm that it has fixed
the bug, or tell me what is still not right.  The file cc-langs.el is in
the directory ..../lisp/progmodes/.

Note that after applying the patch, you need to recompile, at the very
least, cc-langs.el, cc-fonts.el, cc-engine.el, cc-mode.el.  (Or just
recompile all of cc-*.el).


diff -r 2fcfc6e054b3 cc-langs.el
--- a/cc-langs.el	Sun Jul 03 17:54:20 2016 +0000
+++ b/cc-langs.el	Wed Jul 06 11:45:48 2016 +0000
@@ -3233,8 +3233,8 @@
 			  (append (c-lang-const c-label-kwds)
 				  (c-lang-const c-protection-kwds))
 			  :test 'string-equal)))
-  ;; Don't allow string literals, except in AWK.  Character constants are OK.
-  (c objc java pike idl) (concat "\"\\|"
+  ;; Don't allow string literals, except in AWK and Java.  Character constants are OK.
+  (c objc pike idl) (concat "\"\\|"
 				 (c-lang-const c-nonlabel-token-key))
   ;; Also check for open parens in C++, to catch member init lists in
   ;; constructors.  We normally allow it so that macros with arguments


> Emacs  : GNU Emacs 25.1.50.2 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8)
>   of 2016-04-15
> Package: CC Mode 5.33 (Java/l)
> Buffer Style: java
> c-emacs-features: (pps-extended-state col-0-paren posix-char-classes 
> gen-string-delim gen-comment-delim syntax-properties 1-bit)

[ CC Mode state appreciated and snipped. ]

> -- 
> Åsmund Grammeltvedt

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #16 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Jostein Kjønigsen <jostein <at> secure.kjonigsen.net>
To: Alan Mackenzie <acm <at> muc.de>,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Cc: 23901 <at> debbugs.gnu.org
Subject: Re: bug#23901: CC Mode 5.33 (Java/l);
 Wrong indentation of fallthrough switch/case with strings
Date: Wed, 06 Jul 2016 15:56:39 +0200

On Wed, Jul 6, 2016, at 01:55 PM, Alan Mackenzie wrote:
> Hello, Åsmund.
> 
> On Tue, Jul 05, 2016 at 09:37:15PM +0200, Åsmund Grammeltvedt wrote:
>> Hi!
> 
>> Running indent-region on the following code produces wrong indentation
>> on all but the first statement in the last switch block.
> 
> OK.
> 
>> I have included two cases that work correctly for comparison:
> 
> Thank you for such a clear description of the fault, and thanks also for
> including a CC Mode configuration dump (from C-c C-b).
> 
>> class Foo {
>>     public void Bar(String foo) {
>>         int a = 0;
> 
>>         switch (a) {
>>         case 0:
>>         case 1:
>>             a += 1;
>>             a += 2;
>>             break;
>>         }
> 
>>         switch (foo) {
>>         case "foo":
>>             a += 1;
>>             a += 2;
>>             break;
>>         }
> 
>>         switch (foo) {
>>         case "foo":
>>         case "bar":
>>             a += 1;
>>         a += 2;
>>         break;
>>         }
>>     }
>> }
> 
> The problem appears to be in the CC Mode variable
> `c-nonlabel-token-key', which specifies things which can't be in a
> label.  The Java value includes the character '"', presumably from a
> period in ancient history when strings in Java couldn't be case labels.
> 
> Please try out the following fix, and please confirm that it has fixed
> the bug, or tell me what is still not right.  The file cc-langs.el is in
> the directory ..../lisp/progmodes/.
> 
> Note that after applying the patch, you need to recompile, at the very
> least, cc-langs.el, cc-fonts.el, cc-engine.el, cc-mode.el.  (Or just
> recompile all of cc-*.el).
> 
> 
> diff -r 2fcfc6e054b3 cc-langs.el
> --- a/cc-langs.el       Sun Jul 03 17:54:20 2016 +0000
> +++ b/cc-langs.el       Wed Jul 06 11:45:48 2016 +0000
> @@ -3233,8 +3233,8 @@
> (append (c-lang-const c-label-kwds)
> (c-lang-const c-protection-kwds))
> :test 'string-equal)))
> -  ;; Don't allow string literals, except in AWK.  Character constants
> are OK.
> -  (c objc java pike idl) (concat "\"\\|"
> +  ;; Don't allow string literals, except in AWK and Java.  Character
> constants are OK.
> +  (c objc pike idl) (concat "\"\\|"
> (c-lang-const c-nonlabel-token-key))
> ;; Also check for open parens in C++, to catch member init lists in
> ;; constructors.  We normally allow it so that macros with arguments
> 
> 
>> Emacs  : GNU Emacs 25.1.50.2 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8)
>> of 2016-04-15
>> Package: CC Mode 5.33 (Java/l)
>> Buffer Style: java
>> c-emacs-features: (pps-extended-state col-0-paren posix-char-classes
>> gen-string-delim gen-comment-delim syntax-properties 1-bit)
> 
> [ CC Mode state appreciated and snipped. ]
> 
>> --
>> Åsmund Grammeltvedt
> 
> --
> Alan Mackenzie (Nuremberg, Germany).

Hey Alan.

Thanks for the excellent response.

Quick dislaimer: I'm the current csharp-mode maintainer, and this bug
was
originally issued for csharp-mode. As we discovered the same bug was
found
in java-mode which csharp-mode is based on, we suggested reporting the
error
upstream.

I can confirm that this patch does indeed fix the indentation issue,
also in
csharp-mode, but unfortunately it seems to have some side-effects
besides just
indentation.

csharp-mode has a small suite of automated test to verify behaviour,
fontification, indentation, imenu-results, etc.

After applying your patch and recompiling all cc-mode *.el-files, two of
our
tests which used to be OK are now failing. They're both related to
fontification: one for #compiler directives and another for
string-literal
termination.

To me, these kind of side-effects seems a bit unexpected. Would it be OK
to ask
you for some feedback? Basically all you need to reproduce is doing the
following:

> git clone https://github.com/josteink/csharp-mode
> cd csharp-mode
> make clean && make test

I don't want to bother you too much with third-party code, but thought
you may
find it interesting none the less, since this may also affect other
cc-mode
derived modes.

--
Jostein Kjønigsen
jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

Message #19 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: jostein <at> kjonigsen.net
Cc: 23901 <at> debbugs.gnu.org,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l); Wrong indentation of
 fallthrough switch/case with strings
Date: Wed, 6 Jul 2016 21:02:15 +0000

Hello, Jostein.

On Wed, Jul 06, 2016 at 03:56:39PM +0200, Jostein Kjønigsen wrote:
> On Wed, Jul 6, 2016, at 01:55 PM, Alan Mackenzie wrote:

[ .... ]

> > The problem appears to be in the CC Mode variable
> > `c-nonlabel-token-key', which specifies things which can't be in a
> > label.  The Java value includes the character '"', presumably from a
> > period in ancient history when strings in Java couldn't be case labels.

> > Please try out the following fix, and please confirm that it has fixed
> > the bug, or tell me what is still not right.  The file cc-langs.el is in
> > the directory ..../lisp/progmodes/.

> > Note that after applying the patch, you need to recompile, at the very
> > least, cc-langs.el, cc-fonts.el, cc-engine.el, cc-mode.el.  (Or just
> > recompile all of cc-*.el).


> > diff -r 2fcfc6e054b3 cc-langs.el
> > --- a/cc-langs.el       Sun Jul 03 17:54:20 2016 +0000
> > +++ b/cc-langs.el       Wed Jul 06 11:45:48 2016 +0000
> > @@ -3233,8 +3233,8 @@
> > (append (c-lang-const c-label-kwds)
> > (c-lang-const c-protection-kwds))
> > :test 'string-equal)))
> > -  ;; Don't allow string literals, except in AWK.  Character constants
> > are OK.
> > -  (c objc java pike idl) (concat "\"\\|"
> > +  ;; Don't allow string literals, except in AWK and Java.  Character
> > constants are OK.
> > +  (c objc pike idl) (concat "\"\\|"
> > (c-lang-const c-nonlabel-token-key))
> > ;; Also check for open parens in C++, to catch member init lists in
> > ;; constructors.  We normally allow it so that macros with arguments


> >> Emacs  : GNU Emacs 25.1.50.2 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8)
> >> of 2016-04-15
> >> Package: CC Mode 5.33 (Java/l)
> >> Buffer Style: java
> >> c-emacs-features: (pps-extended-state col-0-paren posix-char-classes
> >> gen-string-delim gen-comment-delim syntax-properties 1-bit)

> > [ CC Mode state appreciated and snipped. ]

> >> --
> >> Åsmund Grammeltvedt

> > --
> > Alan Mackenzie (Nuremberg, Germany).

> Hey Alan.

> Thanks for the excellent response.

> Quick dislaimer: I'm the current csharp-mode maintainer, and this bug
> was originally issued for csharp-mode. As we discovered the same bug
> was found in java-mode which csharp-mode is based on, we suggested
> reporting the error upstream.

> I can confirm that this patch does indeed fix the indentation issue,
> also in csharp-mode, but unfortunately it seems to have some
> side-effects besides just indentation.

> csharp-mode has a small suite of automated test to verify behaviour,
> fontification, indentation, imenu-results, etc.

> After applying your patch and recompiling all cc-mode *.el-files, two
> of our tests which used to be OK are now failing. They're both related
> to fontification: one for #compiler directives and another for
> string-literal termination.

> To me, these kind of side-effects seems a bit unexpected. Would it be
> OK to ask you for some feedback? Basically all you need to reproduce is
> doing the following:

Things are rarely that simple.  :-(

> > git clone https://github.com/josteink/csharp-mode
> > cd csharp-mode
> > make clean && make test

On the make test, I get the following messages, the last ones being an
error:

"/usr/local/bin/emacs" -Q -batch -L . -l csharp-mode-tests.el -f
ert-run-tests-batch-and-exit
Contacting host: melpa.org:443
Opening TLS connection to `melpa.org'...
Opening TLS connection with `gnutls-cli --insecure -p 443 melpa.org'...
Opening TLS connection with `gnutls-cli --insecure -p 443
melpa.org'...done
Opening TLS connection to `melpa.org'...done
Saving file /home/acm/.emacs.d/elpa/archives/melpa/archive-contents...
Wrote /home/acm/.emacs.d/elpa/archives/melpa/archive-contents
Contacting host: elpa.gnu.org:80
Saving file /home/acm/.emacs.d/elpa/archives/gnu/archive-contents...
Wrote /home/acm/.emacs.d/elpa/archives/gnu/archive-contents
Package `emacs-24.4' is unavailable
makefile:22: recipe for target 'test' failed
make: *** [test] Error 255

References to a package `emacs-24.4' aren't conducive to a good night's
sleep.  ;-)

> I don't want to bother you too much with third-party code, but thought
> you may find it interesting none the less, since this may also affect
> other cc-mode derived modes.

Certainly, yes.  But maybe it might be simpler if you could describe the
two test cases that fail, and how to reproduce the failures starting from
emacs -Q, loading csharp-mode.  I've got a copy of the csharp-mode
repository, including the testing files.

> --
> Jostein Kjønigsen
> jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #22 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Jostein Kjønigsen <jostein <at> secure.kjonigsen.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 23901 <at> debbugs.gnu.org, Vasilij Schneidermann <v.schneidermann <at> gmail.com>,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l);
 Wrong indentation of fallthrough switch/case with strings
Date: Thu, 07 Jul 2016 10:22:24 +0200

On Wed, Jul 6, 2016, at 11:02 PM, Alan Mackenzie wrote:
> Certainly, yes.  But maybe it might be simpler if you could describe the
> two test cases that fail, and how to reproduce the failures starting from
> emacs -Q, loading csharp-mode.  I've got a copy of the csharp-mode
> repository, including the testing files.
> 
>> --
>> Jostein Kjønigsen
>> jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net
> 
> --
> Alan Mackenzie (Nuremberg, Germany).

Thanks for your reply. I certainly agree about your suggestion.

I think the absolutely simplest approach is to just observe the
fontification of the test-document visually, with and without your patch
applied.

To see how the document -should- be fontified, run Emacs without any
patches or modifications to cc-defs.

Then open csharp-mode.el, and run 'M-x eval-buffer'. csharp-mode should
now be fully loaded and initialised.

After doing this, just try opening the test-document
"test-files/fontification-test.cs".

It should be fairly obvious from the file what correct fontification
should be like.

Now exit Emacs, apply your patch to cc-langs.el, and retry the procedure
outlined above. You should now observe that fontification of the
test-document no longer is applied correctly.

I cannot really see how that patch, in cc-langs.el could affect this
code so directly, but on the other hand it's hard to argue with what you
see.

Any ideas on what's going on here? While "fixing" csharp-mode is
obviously not going to be your job, you may see things happening, and
extrapolate how that may apply to other third-party cc-mode
derivatives... And if your patch should be deemed safe or not.

csharp-mode has a history for having weird hacks, but we're trying to
get rid of them as much as possible. Hopefully this isn't one of those
hacks causing (more) compatibility concerns than a maintainer should
have to worry about...

--
Jostein Kjønigsen
jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

Message #25 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: jostein <at> kjonigsen.net
Cc: 23901 <at> debbugs.gnu.org, Vasilij Schneidermann <v.schneidermann <at> gmail.com>,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l); Wrong indentation of
 fallthrough switch/case with strings
Date: Thu, 7 Jul 2016 12:33:11 +0000

Hello, Jostein.

On Thu, Jul 07, 2016 at 10:22:24AM +0200, Jostein Kjønigsen wrote:
> On Wed, Jul 6, 2016, at 11:02 PM, Alan Mackenzie wrote:
> > Certainly, yes.  But maybe it might be simpler if you could describe the
> > two test cases that fail, and how to reproduce the failures starting from
> > emacs -Q, loading csharp-mode.  I've got a copy of the csharp-mode
> > repository, including the testing files.

> >> --
> >> Jostein Kjønigsen
> >> jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

> > --
> > Alan Mackenzie (Nuremberg, Germany).

> Thanks for your reply. I certainly agree about your suggestion.

> I think the absolutely simplest approach is to just observe the
> fontification of the test-document visually, with and without your patch
> applied.

OK.  Given that the file is small, this seems reasonable.

> To see how the document -should- be fontified, run Emacs without any
> patches or modifications to cc-defs.

I see the same problems regardless of my latest proposed patch to
cc-langs.el.

> Then open csharp-mode.el, and run 'M-x eval-buffer'. csharp-mode should
> now be fully loaded and initialised.

> After doing this, just try opening the test-document
> "test-files/fontification-test.cs".

> It should be fairly obvious from the file what correct fontification
> should be like.

I see that the backslash at the end of the Literal2 string acts as an
escape character, causing the following lines to get mis-fontifified.

> Now exit Emacs, apply your patch to cc-langs.el, and retry the procedure
> outlined above. You should now observe that fontification of the
> test-document no longer is applied correctly.

> I cannot really see how that patch, in cc-langs.el could affect this
> code so directly, but on the other hand it's hard to argue with what you
> see.

> Any ideas on what's going on here? While "fixing" csharp-mode is
> obviously not going to be your job, you may see things happening, and
> extrapolate how that may apply to other third-party cc-mode
> derivatives... And if your patch should be deemed safe or not.

Yes, I have a good idea as to what's going wrong.  The backslash is not
getting marked with a syntax-table text property.  The reason for this
is, I think, that csharp-mode is trying to use
`syntax-propertize-function' to do this.  The problem is that nothing is
triggering a call to this function.  This is one of the reasons that the
CC Mode core doesn't use the `syntax-propertize-function' mechanism - it
is fairly random when and whether it gets run.  Officially it describes
itself as a "lazy" function, as if that were a positive aspect.

I would recommend you to replace csharp-mode-syntax-propertize-function
with a function to be called directly from after-change-functions, and
to place this function in the csharp-mode value of
`c-before-font-lock-functions'.  It should be clear enough from
`c-before-font-lock-functions''s doc string, and from the existing
functions for other modes (including Java Mode) how this should work.  A
similar function to be called from before-change-functions could be
placed on `c-get-state-before-change-functions', but I don't think you
will need this.

> csharp-mode has a history for having weird hacks, but we're trying to
> get rid of them as much as possible. Hopefully this isn't one of those
> hacks causing (more) compatibility concerns than a maintainer should
> have to worry about...

Let's hope not!

> --
> Jostein Kjønigsen
> jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #28 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Jostein Kjønigsen <jostein <at> secure.kjonigsen.net>
To: Alan Mackenzie <acm <at> muc.de>, jostein <at> kjonigsen.net
Cc: 23901 <at> debbugs.gnu.org, Vasilij Schneidermann <v.schneidermann <at> gmail.com>,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l);
 Wrong indentation of fallthrough switch/case with strings
Date: Thu, 07 Jul 2016 15:15:12 +0200

On Thu, Jul 7, 2016, at 02:33 PM, Alan Mackenzie wrote:
> Yes, I have a good idea as to what's going wrong.  The backslash is not
> getting marked with a syntax-table text property.  The reason for this
> is, I think, that csharp-mode is trying to use
> `syntax-propertize-function' to do this.  The problem is that nothing is
> triggering a call to this function.  This is one of the reasons that the
> CC Mode core doesn't use the `syntax-propertize-function' mechanism - it
> is fairly random when and whether it gets run.  Officially it describes
> itself as a "lazy" function, as if that were a positive aspect.
> 
> I would recommend you to replace csharp-mode-syntax-propertize-function
> with a function to be called directly from after-change-functions, and
> to place this function in the csharp-mode value of
> `c-before-font-lock-functions'.  It should be clear enough from
> `c-before-font-lock-functions''s doc string, and from the existing
> functions for other modes (including Java Mode) how this should work.  A
> similar function to be called from before-change-functions could be
> placed on `c-get-state-before-change-functions', but I don't think you
> will need this.
> 
> --
> Alan Mackenzie (Nuremberg, Germany).

Thanks again for your help. It's very much appreciated.

What you say explain a few inconsistencies we've observed in the past,
and I agree with your recommendation about trying to find other
functions/hooks to cover our needs.

If this has not helped you asses whether your patch is "safe" or not,
sorry for wasting your time. :)

We'll see if we can work our way around this, and then hopefully when
your patch lands, our code should be ready for whatever changes it
brings too.

--
Jostein Kjønigsen
jostein <at> kjonigsen.net / jostein <at> secure.kjonigsen.net

Message #31 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l); Wrong indentation of
 fallthrough switch/case with strings
Date: Thu, 7 Jul 2016 15:13:53 +0200

Hello,

> Yes, I have a good idea as to what's going wrong.  The backslash is not
> getting marked with a syntax-table text property.  The reason for this
> is, I think, that csharp-mode is trying to use
> `syntax-propertize-function' to do this.  The problem is that nothing is
> triggering a call to this function.  This is one of the reasons that the
> CC Mode core doesn't use the `syntax-propertize-function' mechanism - it
> is fairly random when and whether it gets run.  Officially it describes
> itself as a "lazy" function, as if that were a positive aspect.

I doubt this is related as I've changed csharp-mode.el to include the
standard definition for c-nonlabel-token-key minus the string delimiter
parts and that made the broken tests work again.  Judging from my
previous experiences with `syntax-propertize-function', unexpected
interactions can arise from  other places, including the use of
`font-lock-syntactic-keywords'.  FWIW, I wouldn't rule out an old
csharp-mode.elc that's still around or an incorrectly compiled cc-mode.

I also doubt that `syntax-propertize-function' is unreliable.  If you
trace from its definition backwards through the sources of
fontification-related files, you'll notice that it is triggered on
initial fontification of a region and after buffer modifications.

Yes, laziness is a positive aspect.  I'd expect it to be self-evident
that avoiding any more work than needed for refontification is important
for performant display and editing of code.  Why would that not apply
for modes derived from cc-mode?

> I would recommend you to replace csharp-mode-syntax-propertize-function
> with a function to be called directly from after-change-functions, and
> to place this function in the csharp-mode value of
> `c-before-font-lock-functions'.  It should be clear enough from
> `c-before-font-lock-functions''s doc string, and from the existing
> functions for other modes (including Java Mode) how this should work.  A
> similar function to be called from before-change-functions could be
> placed on `c-get-state-before-change-functions', but I don't think you
> will need this.

This sounds wrong as font-lock is already doing this in a less ad-hoc
way.  Reusing its API for that solves problems such as having a
specified region to refontify after changes (otherwise one would need to
guess a suitable one with `after-change-functions' or worse, use the
entire buffer).  Why exactly would I want to avoid it?

Cheers
Vasilij

Message #34 received at 23901 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l); Wrong indentation of
 fallthrough switch/case with strings
Date: Thu, 7 Jul 2016 18:33:16 +0000

Hello, Vasilij.

On Thu, Jul 07, 2016 at 03:13:53PM +0200, Vasilij Schneidermann wrote:
> Hello,

> > Yes, I have a good idea as to what's going wrong.  The backslash is not
> > getting marked with a syntax-table text property.  The reason for this
> > is, I think, that csharp-mode is trying to use
> > `syntax-propertize-function' to do this.  The problem is that nothing is
> > triggering a call to this function.  This is one of the reasons that the
> > CC Mode core doesn't use the `syntax-propertize-function' mechanism - it
> > is fairly random when and whether it gets run.  Officially it describes
> > itself as a "lazy" function, as if that were a positive aspect.

> I doubt this is related as I've changed csharp-mode.el to include the
> standard definition for c-nonlabel-token-key minus the string delimiter
> parts and that made the broken tests work again.  Judging from my
> previous experiences with `syntax-propertize-function', unexpected
> interactions can arise from  other places, including the use of
> `font-lock-syntactic-keywords'.

These unexpected interactions are another reason CC Mode doesn't use
`syntax-propertize-function'.

> FWIW, I wouldn't rule out an old csharp-mode.elc that's still around
> or an incorrectly compiled cc-mode.

Well, I used an interpreted csharp-mode.el, and I think I know quite a
bit about correctly compiling CC Mode.  ;-)

> I also doubt that `syntax-propertize-function' is unreliable.

Of itself, it is indeed reliable.  But something has to call it, and
that part is not reliable.  It's arbitrary and random.

> If you trace from its definition backwards through the sources of
> fontification-related files, you'll notice that it is triggered on
> initial fontification of a region and after buffer modifications.

Only when fontification is enabled.  Even then, after M-x csharp-mode,
a critical character in the .../test-files/fontification-test.cs (from
the csharp-mode repository) didn't have a syntax-table property on it.
After he first buffer change, it acquired that property.

Don't forget, font-locking doesn't own syntax-table properties.  These
properties are needed for other reasons too.

> Yes, laziness is a positive aspect.  I'd expect it to be self-evident
> that avoiding any more work than needed for refontification is important
> for performant display and editing of code.  Why would that not apply
> for modes derived from cc-mode?

Because the work has to be done at some stage.  The
`syntax-propertize-function' mechanism, at a buffer change, wastefully
erases all syntax-table properties from that point in the buffer
onwards, meaning they have to be recalculated.  In CC Mode, these
properties are kept up to date directly in
before/after-change-functions, and positions distant from point do not
have their properties smashed, minimising the work done.

> > I would recommend you to replace csharp-mode-syntax-propertize-function
> > with a function to be called directly from after-change-functions, and
> > to place this function in the csharp-mode value of
> > `c-before-font-lock-functions'.  It should be clear enough from
> > `c-before-font-lock-functions''s doc string, and from the existing
> > functions for other modes (including Java Mode) how this should work.  A
> > similar function to be called from before-change-functions could be
> > placed on `c-get-state-before-change-functions', but I don't think you
> > will need this.

> This sounds wrong as font-lock is already doing this in a less ad-hoc
> way.

Font-lock can only do this when it is enabled.  It is a goal of CC Mode
also to work when font-lock is disabled.  Font lock can only set these
properties during redisplay, which is too late for some uses of them.
(For example, when you type in a ">", that instantly matches an opening
"<", the determination being done before redisplay.)

CC Mode's mechanism here is not "ad-hoc".  It is direct and logical.

> Reusing its API for that solves problems such as having a specified
> region to refontify after changes (otherwise one would need to guess a
> suitable one with `after-change-functions' or worse, use the entire
> buffer).

Font lock on its own cannot determine the region to fontify.  It needs
input from the major mode in the general case.  Font lock has the
requisite interfaces for this, and anyhow this point has nothing to do
with the use/non-use of `syntax-propertize-function'.

> Why exactly would I want to avoid it?

Because of the reasons identified by both of us in these posts:  The
unwanted interactions `syntax-propertize-function' has with other parts
of Emacs;  the requirement to work when Font lock is disabled;  the fact
it is not working properly at the moment in csharp-mode.el;  the extra
run-time the laziness and unneeded erasure of text properties causes.

> Cheers
> Vasilij

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #39 received at 23901-done <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: 23901-done <at> debbugs.gnu.org
Cc: Jostein Kjønigsen <jostein <at> secure.kjonigsen.net>,
 Åsmund Grammeltvedt <asmundg <at> big-oil.org>
Subject: Re: bug#23901: CC Mode 5.33 (Java/l); Wrong indentation of
 fallthrough switch/case with strings
Date: Sat, 23 Jul 2016 10:55:07 +0000

Bug fixed in master.

-- 
Alan Mackenzie (Nuremberg, Germany).

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.