public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: "Manuel López-Ibáñez" <lopezibanez@gmail.com>
To: Mark Mitchell <mark@codesourcery.com>
Cc: Gcc Patch List <gcc-patches@gcc.gnu.org>,
	Jason Merrill <jason@redhat.com>,
	 	Nathan Sidwell <nathan@codesourcery.com>,
	Janis Johnson <janis187@us.ibm.com>
Subject: Re: [C/C++] PR 13358 long long and C++ do not mix well
Date: Mon, 20 Apr 2009 16:17:00 -0000	[thread overview]
Message-ID: <6c33472e0904200917q221b7e76hd4aeedfe164b8f4e@mail.gmail.com> (raw)
In-Reply-To: <49EC9148.9070806@codesourcery.com>

2009/4/20 Mark Mitchell <mark@codesourcery.com>:
> Manuel López-Ibáñez wrote:
>
> which matches anything, and which I agree is sloppy (though I added
> plenty such tests over the years) and putting the entire text there.
> It's generally possible to pick out a few key words from the error
> message ("invalid overload" or "does not match" or "private" or some
> such) that are the important part of the error message.
>

If one changes a message and there are not testsuite changes, then one
cannot know whether this message is actually ever tested. If no
testcase is added, then the feature may go untested. If one adds
another testcase, then we may have two equivalent testcases for the
same thing. The latter happens already in the g++.dg and g++.old-deja
dirs.

If there is one important keyword only, your proposal is reasonable
but if there are several important keywords, then the choices are to
use "\[^\n\]*", which is ugly to read and awful to search/replace, or
use "|", which matches more than we want to match.

For example, if you look at this pattern:

-  i = 18446744073709551615; /* { dg-warning "decimal
constant|unsigned" "decimal constant" } */

you cannot tell which error message is matched. In fact, it turns out
that there are two messages for this line:

+  i = 18446744073709551615; /* { dg-warning "integer constant is so
large that it is unsigned" "decimal constant" } */
+  /* { dg-warning "this decimal constant would be unsigned in ISO
C90" "decimal constant" { target *-*-* } 28 } */

One mentions C90 (but it will still pass if it mentioned C++ or C++0x
or pascal) and the other not. In this case, probably the first message
is redundant but if it were important, we can lose it and it will
never be noticed. We could also lose the second message and the test
will still pass. On the other hand, perhaps the intention was to match
only the second message and the redundant one may have appeared
without anyone noticing.

Moreover, I just noticed that in the g++ testcase I am using:

+  x1 = 0x1b27da572ef3cd86LL; // { dg-warning "long long" }

which is not actually testing that CPP is printing:

     warning: use of C++0x long long integer constant

instead of:

    warning: use of C99 long long integer constant

So this feature may regress in the future without anyone noticing.

>> Nonetheless, you are the maintainer, so if you want me to cut the test
>> pattern, tell me what I should match and I will update the patch.
>
> I'm not going to make you change a perfectly good patch just for this.
> But, unless Jason/Nathan indicate otherwise, I'd appreciate it if for
> future patches you take a more circumspect approach.

OK, I'll try, less work for me. But if I err on the side of
specificity rather than generality, feel free to ask for changes.

Cheers,

Manuel.

  reply	other threads:[~2009-04-20 16:17 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-30  1:11 Manuel López-Ibáñez
2008-08-30  2:15 ` Joseph S. Myers
2008-08-30  9:52   ` Manuel López-Ibáñez
2008-10-23  2:42     ` Manuel López-Ibáñez
2009-04-10 19:12       ` Manuel López-Ibáñez
2009-04-10 21:41         ` Joseph S. Myers
2009-04-19 11:16           ` Manuel López-Ibáñez
2009-04-20  3:34             ` Mark Mitchell
2009-04-20  8:38               ` Manuel López-Ibáñez
2009-04-20 15:14                 ` Mark Mitchell
2009-04-20 16:17                   ` Manuel López-Ibáñez [this message]
2009-04-20 16:26                     ` Mark Mitchell
2009-04-20 17:38                       ` Jason Merrill
2009-04-23  9:15                         ` Dave Korn
2009-04-23 23:46                           ` Mark Mitchell
2009-04-24 13:39                             ` Dave Korn

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=6c33472e0904200917q221b7e76hd4aeedfe164b8f4e@mail.gmail.com \
    --to=lopezibanez@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=janis187@us.ibm.com \
    --cc=jason@redhat.com \
    --cc=mark@codesourcery.com \
    --cc=nathan@codesourcery.com \
    /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.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).