Re: warning: multi-line comment (why?)

Re: warning: multi-line comment (why?)

[Joern Rennecke]

> You get diagnostics as reqd by the standard.

Is that with or without a colomn number?

Re: warning: multi-line comment (why?)

[Neil Booth]

Joern Rennecke wrote:-
> > You get diagnostics as reqd by the standard.
> 
> Is that with or without a colomn number?

Ah, now I see what you're getting at.  The \\n case reports the
beginning of the in-progress token; missing \n reports the col number.
Not sure it's worth changing; it's not as if the location is in any
doubt.  Do you think it's worth the extra overhead to get a correctly
reported column number for unexpected EOF?

Neil.

Re: warning: multi-line comment (why?)

[Joern Rennecke]

> Ah, now I see what you're getting at.  The \\n case reports the
> beginning of the in-progress token; missing \n reports the col number.
> Not sure it's worth changing; it's not as if the location is in any
> doubt.  Do you think it's worth the extra overhead to get a correctly
> reported column number for unexpected EOF?

I don't know.  But we should be aware that we are deciding about this.
I think it's OK if you leave out the coloumn number when you realise that
you don't know it, but it is dubious to report an incorrect one.

Re: warning: multi-line comment (why?)

[Jamie Lokier]

Neil Booth wrote:
> Hmm, you didn't read my mail did you? :-) You've not really fixed it,
> and made it hard work.  What if there's two escaped newlines in a row?
> Or if there's a trigraphed backslash?  get_effective_char handles all
> that for you, rather than you checking the result of a buffer->cur++.

You didn't read my code did you? :-)
It works fine with multiple escaped newlines and trigraphs.

Read the code carefully.  It doesn't do *buffer->cur++.  It does
*buffer->cur.  Why?  See below.

> To be consistent we should also whitespace-skip \v and \f.

The coding is deliberate, though you may disagree with my choice.

I want to suppress the warning in the case of "// something
\<newline><hspace>// something else", but for all other cases the source
is quite unusual.  In those cases, keep the warning.  That includes \v,
\f and escaped newlines in the comment delimiter.

The test case illustrates that.

Btw, the code is pretty fast.  The extra code is only executed in these
unusual cases -- the normal comment parsing code sticks to the early
part of the loop and exits quickly at the end of the line.

>  No special tab handling is needed - it's only there to keep track of
> columns for warnings, and since we're a line comment there are not
> going to be any warnings on the line (trigraph warnings are now turned
> off).  is_nvspace does all that for you.
...
> Ah, now I see what you're getting at.  The \\n case reports the
> beginning of the in-progress token; missing \n reports the col number.
> Not sure it's worth changing; it's not as if the location is in any
> doubt.  Do you think it's worth the extra overhead to get a correctly
> reported column number for unexpected EOF?

The code to track the column number is very, very rarely called.  You
have to have an escaped newline at the end of a // comment, followed by
tab characters to reach that code.  How likely is that?  So forget about
the overhead; there isn't one.

enjoy,
-- Jamie

Re: warning: multi-line comment (why?)

[Neil Booth]

Jamie Lokier wrote:-

> You didn't read my code did you? :-)

Not closely enough :)

> The coding is deliberate, though you may disagree with my choice.

OK, I'm not of a strong opinion.

> The code to track the column number is very, very rarely called.  You
> have to have an escaped newline at the end of a // comment, followed by
> tab characters to reach that code.  How likely is that?  So forget about
> the overhead; there isn't one.

Yes.

Neil.

warning: multi-line comment (why?)

[Craig Rodrigues]

Hi,

I am running:
gcc version 2.96 20000917 (experimental) 

under x86 Redhat Linux.  I obtained the snapshot from Codesourcery.

If I have the following code:
====================================================================
//  Some comment
//  \
//  Some more comment
 
int main(int, char **) { return 0; }       
====================================================================

compiling with g++ will yield the following warning:
x.cc:2:2: warning: multi-line comment 

What is the point of this warning?  Shouldn't the preprocessor
just ignore everything between the // and the end-of-line?

Thanks.

-- 
Craig Rodrigues        
http://www.gis.net/~craigr    
rodrigc@mediaone.net          

Re: warning: multi-line comment (why?)

[Jan Dvorak]

On Sun, Sep 17, 2000 at 06:54:53PM -0400, Craig Rodrigues wrote:
> If I have the following code:
> ====================================================================
> //  Some comment
> //  \
      ^^
> //  Some more comment
>  

char '\' at end of line is threaten as that statement is continuing on next
line. '//' comments is mentioned for one line only, and for multiline
comments the right way is '/* */'.

And the warning comes from preprocesor intself - try 'cpp -Wall ./source.c'.

Jan Dvorak <johnydog@go.cz>

Re: warning: multi-line comment (why?)

[Richard Henderson]

On Sun, Sep 17, 2000 at 06:54:53PM -0400, Craig Rodrigues wrote:
> x.cc:2:2: warning: multi-line comment 
> 
> What is the point of this warning?  Shouldn't the preprocessor
> just ignore everything between the // and the end-of-line?

No, backslash-newline conversion happens before comments
are discarded.  You really do have a multi-line comment;
one that would be dangerous if your next line weren't a
comment as well.


r~

Re: warning: multi-line comment (why?)

[Jamie Lokier]

Richard Henderson wrote:
> > What is the point of this warning?  Shouldn't the preprocessor
> > just ignore everything between the // and the end-of-line?
> 
> No, backslash-newline conversion happens before comments
> are discarded.  You really do have a multi-line comment;
> one that would be dangerous if your next line weren't a
> comment as well.

But the next line _is_ a comment so the warning is inappropriate here.

If this were a very unlikely situation, fair enough.  But people do
comment out multi-line macros from time to time using `//', and those
will trigger the warning.

It can't be too hard to skip whitespace and then check for `//' on the
next line I'm sure.

-- Jamie

Re: warning: multi-line comment (why?)

[Jan Dvorak]

On Mon, Sep 18, 2000 at 12:04:19PM +0200, Jamie Lokier wrote:
> Richard Henderson wrote:
> > > What is the point of this warning?  Shouldn't the preprocessor
> > > just ignore everything between the // and the end-of-line?
> > 
> > No, backslash-newline conversion happens before comments
> > are discarded.  You really do have a multi-line comment;
> > one that would be dangerous if your next line weren't a
> > comment as well.
> 
> But the next line _is_ a comment so the warning is inappropriate here.
>
> If this were a very unlikely situation, fair enough.  But people do
> comment out multi-line macros from time to time using `//', and those
> will trigger the warning.

If you comment out macros during development, you may ignore this warning.
If you need commented macro in release, you have to comment it by /**/.

Multiline comment are dangerous, and even if next line begins with '//', it
is not the right way.

> It can't be too hard to skip whitespace and then check for `//' on the
> next line I'm sure.

I think it can be difficult. Referring to cpplex.c:

/* Skip a C++ line comment.  Handles escaped newlines.  Returns
   non-zero if a multiline comment.  The following new line, if any,
   is left in buffer->read_ahead.  */
static int
skip_line_comment (buffer)
     cpp_buffer *buffer;
{

...
      	return orig_lineno != buffer->lineno;
}

so you'll need lot of readahead and lot of work to get it working right.

Jan Dvorak <johnydog@go.cz>

Re: warning: multi-line comment (why?)

[Jamie Lokier]

Jan Dvorak wrote:
> I think it can be difficult. Referring to cpplex.c:
> 
> /* Skip a C++ line comment.  Handles escaped newlines.  Returns
>    non-zero if a multiline comment.  The following new line, if any,
>    is left in buffer->read_ahead.  */
...
> so you'll need lot of readahead and lot of work to get it working right.

No readahead -- if there is an escaped newline, that function already
reads and discards the following line.

See patch below.  How do I submit this?

-- Jamie

diff -u -w -r1.7827 ChangeLog
--- ChangeLog	2000/09/19 16:43:35	1.7827
+++ ChangeLog	2000/09/19 17:08:09
@@ -1,0 +1,6 @@
+2000-09-19  Jamie Lokier  <jamie.lokier@cern.ch>
+
+	* cpplex.c (skip_line_comment): Don't warn about multi-line
+	comments if the following line is another "//" comment.  Arg
+	changed from cpp_buffer* to cpp_reader*.
+
diff -u -w -r1.98 cpplex.c
--- cpplex.c	2000/09/18 18:43:04	1.98
+++ cpplex.c	2000/09/19 17:08:10
@@ -103,7 +103,7 @@
 static cppchar_t get_effective_char PARAMS ((cpp_buffer *));
 
 static int skip_block_comment PARAMS ((cpp_reader *));
-static int skip_line_comment PARAMS ((cpp_buffer *));
+static int skip_line_comment PARAMS ((cpp_reader *));
 static void adjust_column PARAMS ((cpp_reader *));
 static void skip_whitespace PARAMS ((cpp_reader *, cppchar_t));
 static cpp_hashnode *parse_identifier PARAMS ((cpp_reader *, cppchar_t));
@@ -816,14 +816,17 @@
   return c != '/' || prevc != '*';
 }
 
-/* Skip a C++ line comment.  Handles escaped newlines.  Returns
-   non-zero if a multiline comment.  The following new line, if any,
-   is left in buffer->read_ahead.  */
+/* Skip a C++ line comment.  Handles escaped newlines.  Returns non-zero
+   if a multiline comment that we should warn about.  That means there
+   is an escaped newline that is not followed by whitespace then "//".
+   The following new line, if any, is left in buffer->read_ahead.  */
 static int
-skip_line_comment (buffer)
-     cpp_buffer *buffer;
+skip_line_comment (pfile)
+     cpp_reader *pfile;
 {
+  cpp_buffer *buffer = pfile->buffer;
   unsigned int orig_lineno = buffer->lineno;
+  int multiline = 0;
   cppchar_t c;
 
   do
@@ -833,13 +836,36 @@
 	break;
 
       c = *buffer->cur++;
-      if (c == '?' || c == '\\')
+      if (c != '?' && c != '\\')
+	continue;
+
 	c = skip_escaped_newlines (buffer, c);
+      if (orig_lineno == buffer->lineno)
+	continue;
+      orig_lineno = buffer->lineno;
+
+      /* If c == EOF there'll be a warning about backslash-newline at
+	 end of the file.  No need for a multi-line warning as well. */
+      if (c == EOF)
+	continue;
+
+      /* If there is a newline and it is followed by whitespace then
+	 "//", don't warn about that as a multi-line comment. */
+      if (c == ' ' || c == '\t' || c == '\0')
+	{
+	  skip_whitespace (pfile, c);
+	  c = buffer->read_ahead;
+	}
+      if (c == '/' && buffer->cur < buffer->rlimit
+	  && (c = *buffer->cur++) == '/')
+	continue;
+
+      multiline = 1;
     }
   while (!is_vspace (c));
 
   buffer->read_ahead = c;	/* Leave any newline for caller.  */
-  return orig_lineno != buffer->lineno;
+  return multiline;
 }
 
 /* pfile->buffer->cur is one beyond the \t character.  Update
@@ -1342,7 +1368,7 @@
 	      comment_start = buffer->cur;
 
 	      /* Skip_line_comment updates buffer->read_ahead.  */
-	      if (skip_line_comment (buffer))
+	      if (skip_line_comment (pfile))
 		cpp_warning_with_line (pfile, result->line, result->col,
 				       "multi-line comment");
 

Re: warning: multi-line comment (why?)

[Jan Dvorak]

On Tue, Sep 19, 2000 at 07:12:21PM +0200, Jamie Lokier wrote:
> Jan Dvorak wrote:
> > so you'll need lot of readahead and lot of work to get it working right.
> 
> No readahead -- if there is an escaped newline, that function already
> reads and discards the following line.
Sorry, i didn't realize this.

> See patch below.  How do I submit this?

looks fine to me. The only question for now is what other things are against
allowing multiline C++-like comments ? If none, i'm for adding it.

Jan Dvorak <johnydog@go.cz>

Re: warning: multi-line comment (why?)

[Jamie Lokier]

The motivation is for when you do "comment-region" in Emacs to comment
out code that includes macros.

Neil Booth wrote:
> > Regardless, just glancing at it, you've broken silently ignoring nulls
> > in comments.  You've also broken backslash and trigraph-escaped
> > newlines, I think.  I want those fixed, cleanly, and testcases at
> > least.
> 
> Sorry, ignore that about newlines, I meant slashes and I don't think
> I'm right.

Hmm.  The nulls which warn are part of the multi-line comment.  Fair
enough, I'll fix that and get the correct column numbers too.

Trigraph-escaped slashes aren't checked for, so you still get the
warning in that case.  Do you care?

-- Jamie

Re: warning: multi-line comment (why?)

[Neil Booth]

Jamie Lokier wrote:-
> The motivation is for when you do "comment-region" in Emacs to comment
> out code that includes macros.

Hmm, OK you've persuaded me.

> Hmm.  The nulls which warn are part of the multi-line comment.  Fair
> enough, I'll fix that and get the correct column numbers too.

A tight while loop with is_nvspace is enough; don't bother with
skip_whitespace - it does too much.

> Trigraph-escaped slashes aren't checked for, so you still get the
> warning in that case.  Do you care?

Those warnings are not meant to be there; they're just on for about 48
hours between patches :) I'll commit a patch RSN that turns them off
in comments.  What I meant was this:-

// this \
/\
/ is all one comment.

I know it's pedantic, but it's valid and we should continue to get it
right.  I think your loop will terminate the comment at the end of the
2nd line.  A call to get_effective_char will handle that (instead of
buffer->cur++).

If you do this, would you provide a testcase for the bug above, and
that ensures the warning about multiline comments in not emitted in
the case you're talking about?  Don't bother about null character
testcases; in my experience they really confuse CVS.

Thanks,

Neil.

Re: warning: multi-line comment (why?)

[Jamie Lokier]

Neil Booth wrote:
> // this \
> /\
> / is all one comment.
> 
> I know it's pedantic, but it's valid and we should continue to get it
> right.  I think your loop will terminate the comment at the end of the
> 2nd line.  A call to get_effective_char will handle that (instead of
> buffer->cur++).
> 
> If you do this, would you provide a testcase for the bug above, and
> that ensures the warning about multiline comments in not emitted in
> the case you're talking about?  Don't bother about null character
> testcases; in my experience they really confuse CVS.

Neal, you're right my loop was broken with /\<newline>/.  Please find
attached one patch and one test case.  I don't have Dejagnu installed
but I've formatted the test case that way so you can give it a try.

If you like the patch I'll need to send in the copyright assignment
form.

enjoy,
-- Jamie

2000-09-25  Jamie Lokier  <jamie.lokier@cern.ch>

	* cpplex.c (skip_line_comment): Don't warn about multi-line
	comments if the following line is another "//" comment.

Index: cpplex.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/cpplex.c,v
retrieving revision 1.101
diff -u -w -r1.101 cpplex.c
--- cpplex.c	2000/09/24 10:42:09	1.101
+++ cpplex.c	2000/09/25 19:39:52
@@ -796,15 +796,17 @@
   return c != '/' || prevc != '*';
 }
 
-/* Skip a C++ line comment.  Handles escaped newlines.  Returns
-   non-zero if a multiline comment.  The following new line, if any,
-   is left in buffer->read_ahead.  */
+/* Skip a C++ line comment.  Handles escaped newlines.  Returns non-zero
+   if a multiline comment that we should warn about.  That means there
+   is an escaped newline that is not followed by whitespace then "//".
+   The following new line, if any, is left in buffer->read_ahead.  */
 static int
 skip_line_comment (pfile)
      cpp_reader *pfile;
 {
   cpp_buffer *buffer = pfile->buffer;
   unsigned int orig_lineno = buffer->lineno;
+  int multiline = 0;
   cppchar_t c;
 
   pfile->state.lexing_comment = 1;
@@ -815,14 +817,43 @@
 	break;
 
       c = *buffer->cur++;
-      if (c == '?' || c == '\\')
+      if (c != '?' && c != '\\')
+	continue;
+
 	c = skip_escaped_newlines (buffer, c);
+      if (orig_lineno == buffer->lineno)
+	continue;
+      orig_lineno = buffer->lineno;
+
+      /* If c == EOF there'll be a warning about backslash-newline at
+	 end of the file.  No need for a multi-line warning as well. */
+      if (c == EOF)
+	continue;
+
+      /* Skip only ' ' and \t, on the subsequent comment line. */
+      while (is_hspace (c))
+	{
+	  if (c == '\t')
+	    adjust_column (pfile);
+	  c = EOF;
+	  if (buffer->cur == buffer->rlimit)
+	    break;
+	  c = *buffer->cur++;
+	}
+
+      /* If the next token isn't literally "//", warn about a multi-line
+	 comment.  Take care with "/\<newline>/": do warn about
+	 multi-line in this case, and do continue the comment. */
+      if (c == '/' && buffer->cur < buffer->rlimit && *buffer->cur == '/')
+	buffer->cur++;
+      else
+	multiline = 1;
     }
   while (!is_vspace (c));
 
   pfile->state.lexing_comment = 0;
   buffer->read_ahead = c;	/* Leave any newline for caller.  */
-  return orig_lineno != buffer->lineno;
+  return multiline;
 }
 
 /* pfile->buffer->cur is one beyond the \t character.  Update


---------------------
testsuite/gcc.dg/cpp/multiline.c (how do you make a cvs-style diff
against a file which isn't in the repository?)

/* { dg-do compile } */

/* Warn about multi-line //-style comments, but not when the subsequent
   lines are also //-style comments.  Contributed by Jamie Lokier. */

/* { dg-warning "multi-line comment" 7 } */
// this \
  produces a warning;

// neither \
  // this

// nor \
//   this do, though

/* { dg-warning "multi-line comment" 18 } */
int
// this one \
  compiles
  ok;

/* { dg-warning "multi-line comment" 23 } */
// remember to handle \
/\
/ embedded newlines.

Re: warning: multi-line comment (why?)

[Neil Booth]

Jamie Lokier wrote:-

> Neal, you're right my loop was broken with /\<newline>/.  Please find
> attached one patch and one test case.  I don't have Dejagnu installed
> but I've formatted the test case that way so you can give it a try.

Hmm, you didn't read my mail did you? :-) You've not really fixed it,
and made it hard work.  What if there's two escaped newlines in a row?
Or if there's a trigraphed backslash?  get_effective_char handles all
that for you, rather than you checking the result of a buffer->cur++.

To be consistent we should also whitespace-skip \v and \f.  No special
tab handling is needed - it's only there to keep track of columns for
warnings, and since we're a line comment there are not going to be any
warnings on the line (trigraph warnings are now turned off).
is_nvspace does all that for you.

> If you like the patch I'll need to send in the copyright assignment
> form.

I'll do it myself properly soon, and that'll solve the copyright
problem.  I'll cc you when I do.  I think we can use your test case
without problems.

Thanks,

Neil.

Re: warning: multi-line comment (why?)

[Joern Rennecke]

> To be consistent we should also whitespace-skip \v and \f.  No special
> tab handling is needed - it's only there to keep track of columns for
> warnings, and since we're a line comment there are not going to be any
> warnings on the line (trigraph warnings are now turned off).
> is_nvspace does all that for you.

What happens when the line is the last line in the file and is missing
a newline?
What happens if it's the last line and it ends with a backslash / newline?