tpope / vim-commentary

commentary.vim: comment stuff out
http://www.vim.org/scripts/script.php?script_id=3695
5.9k stars 214 forks source link

C multi-line comments #119

Closed natemaia closed 2 years ago

natemaia commented 4 years ago

Currently if you have a comment

if (test) {
    /* A comment
    * Some text
    */
    func();
}

When commenting the block using gca{ it results in a broken section

/* if (test) {*/
/*     /* A comment*/
/*     * Some text*/
/*     */*/               <---- this line
/*     func();*/
/* }*/

I realize this may be asking too much and I should just stick to single line comments or blocking them off myself. Either way thank you for all the plugins you've made over the years.

Cheers, Nate

tpope commented 4 years ago

I recently tweaked that. Does it work if you go back one commit?

natemaia commented 4 years ago

No, I also tried out a several other commits going back in history and it's consistently the same.

One thing I noticed

/*
* this works
* */

after commenting
/*/1* */
/** this works */
/** *1/ */

So I had a little look at the plugin and made this edit which seems to do it, I'm sure there's a cleaner way though I'm just shit with vim regex.

diff --git a/plugin/commentary.vim b/plugin/commentary.vim
index 17c285b..859119a 100644
--- a/plugin/commentary.vim
+++ b/plugin/commentary.vim
@@ -52,15 +52,15 @@ function! s:go(...) abort

   for lnum in range(lnum1,lnum2)
     let line = getline(lnum)
-    if strlen(r) > 2 && l.r !~# '\\'
+    if strlen(r) >= 2 && l.r !~# '\\'
       let line = substitute(line,
             \'\M' . substitute(l, '\ze\S\s*$', '\\zs\\d\\*\\ze', '') . '\|' . substitute(r, '\S\zs', '\\zs\\d\\*\\ze', ''),
-            \'\=substitute(submatch(0)+1-uncomment,"^0$\\|^-\\d*$","","")','g')
+            \'\=substitute(submatch(0)+1-uncomment,"^0$\\|^-\\d*\s*$","","")','g')
     endif
     if uncomment
       let line = substitute(line,'\S.*\s\@<!','\=submatch(0)[strlen(l):-strlen(r)-1]','')
     else
-      let line = substitute(line,'^\%('.matchstr(getline(lnum1),indent).'\|\s*\)\zs.*\S\@<=','\=l.submatch(0).r','')
+      let line = substitute(line,'^\%('.matchstr(getline(lnum1),indent).'\|\s*\)\zs.*\S\@<=','\=l.submatch(0)." ".r','')
     endif
     call setline(lnum,line)
   endfor
tpope commented 4 years ago

Are you setting a custom b:commentary_format? I just noticed your */ lacks whitespace padding.

natemaia commented 4 years ago

No, but I do have a few settings that may be related but after removing them it's still the same result, though I do get the whitespace padding, just due to

setlocal comments=sl:/*,mb:*,elx:*/ 
natemaia commented 4 years ago

With stock commentstring and no plugin changes

before
/* some text
 * more
 */

after (still wrong)
/* /* some text*/
/*  * more*/
/*  */*/
tpope commented 4 years ago

I can reproduce the lack of whitespace padding if one of the lines being commented out contains a */. Everything works fine if I go all the way back to 1.3. Can you confirm the same? If so, a bisect would be helpful.

natemaia commented 4 years ago

Yea 1.3 was good here as well, here's the bisect between master and v1.3.

89f43af18692d22ed999c3097e449f12fdd8b299 is the first bad commit
commit 89f43af18692d22ed999c3097e449f12fdd8b299
Author: Chaoren Lin <chaoren@users.noreply.github.com>
Date:   Mon Oct 9 23:18:52 2017 -0400

    Fix uncommenting with irregular white spaces (#82)

    Allow stripping white spaces on the left and right independently.
    Also, make sure the stripping is not reverted by subsequent lines
    which do have white spaces.

    The following cases were broken and is now fixed:

    Before this change:

    1. ^//foo$    -> ^oo$
       ^// bar$      ^bar$
    2. ^/*foo */$ -> ^foo $
    3. ^/* foo*/$ -> ^/* /1* foo*/ */$

    After this change:

    1. ^//foo$    -> ^foo$
       ^// bar$      ^ bar$
    2. ^/*foo */$ -> ^foo$
    3. ^/* foo*/$ -> ^foo$

 plugin/commentary.vim | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
chrisbra commented 4 years ago

isn't that the same as #92? I made a fix once, not sure if it still applies #93

natemaia commented 4 years ago

I don't believe so

carlescufi commented 2 years ago

With stock commentstring and no plugin changes

before
/* some text
 * more
 */

after (still wrong)
/* /* some text*/
/*  * more*/
/*  */*/

I get the exact same behavior myself on the current latest release, 1.3. Any chance for a fix @tpope? I really appreciate the minimalist approach when compared to other similar plugins, but the incorrect behavior with C block comments is a real issue for us low-level programmers. @natemaia bisected it here.

carlescufi commented 2 years ago

Similarly, it'd be great to be able to configure line vs block comment for C files:

int foo;
foo = bar();

when running gcap you would be able to select between:

/* int foo; */
/* foo = bar(); */

and

/* int foo;
foo = bar(); */

I can instead create a new GitHub issue with this, any preferences?

tpope commented 2 years ago

Similarly, it'd be great to be able to configure line vs block comment for C files:

int foo;
foo = bar();

when running gcap you would be able to select between:

/* int foo; */
/* foo = bar(); */

and

/* int foo;
foo = bar(); */

I can instead create a new GitHub issue with this, any preferences?

This gets super complicated if you want to support things like partial uncommenting. We can't just operate on the given line range; we have to parse the entire file. I don't see this happening.

carlescufi commented 2 years ago

With stock commentstring and no plugin changes

before
/* some text
 * more
 */

after (still wrong)
/* /* some text*/
/*  * more*/
/*  */*/

I get the exact same behavior myself on the current latest release, 1.3. Any chance for a fix @tpope? I really appreciate the minimalist approach when compared to other similar plugins, but the incorrect behavior with C block comments is a real issue for us low-level programmers. @natemaia bisected it here.

Thanks @tpope for the commit, but this is still not functional for me. This is what I get:

        /*
         * Make lookup to check if there's a connection object in
         * CONNECT or CONNECT_AUTO state associated with passed peer LE address.
         */

Visual select the paragraph and then gc:

        /*/1* */
        /* * Make lookup to check if there's a connection object in */
        /* * CONNECT or CONNECT_AUTO state associated with passed peer LE address. */
        /* *1/ */

Maybe I misunderstood what this commit you pushed was fixing?

carlescufi commented 2 years ago

This gets super complicated if you want to support things like partial uncommenting. We can't just operate on the given line range; we have to parse the entire file. I don't see this happening.

Understood, thanks for the feedback.

tpope commented 2 years ago

With stock commentstring and no plugin changes

before
/* some text
 * more
 */

after (still wrong)
/* /* some text*/
/*  * more*/
/*  */*/

I get the exact same behavior myself on the current latest release, 1.3. Any chance for a fix @tpope? I really appreciate the minimalist approach when compared to other similar plugins, but the incorrect behavior with C block comments is a real issue for us low-level programmers. @natemaia bisected it here.

Thanks @tpope for the commit, but this is still not functional for me. This is what I get:

        /*
         * Make lookup to check if there's a connection object in
         * CONNECT or CONNECT_AUTO state associated with passed peer LE address.
         */

Visual select the paragraph and then gc:

        /*/1* */
        /* * Make lookup to check if there's a connection object in */
        /* * CONNECT or CONNECT_AUTO state associated with passed peer LE address. */
        /* *1/ */

Maybe I misunderstood what this commit you pushed was fixing?

This is the intended behavior. The old result had a syntax error at /* */*/. This one doesn't, and is still reversible.

It's not trying to remove an arbitrary multi-line comment. That's back to needing to parse the entire file.

carlescufi commented 2 years ago

This is the intended behavior. The old result had a syntax error at /* */*/. This one doesn't, and is still reversible.

Right, as I suspected. Thanks for confirming!