This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: UCNs-in-IDs patch
On Sun, 13 Mar 2005, Mark Mitchell wrote:
> I'm all for people building a very high-quality free testsuite, but I don't
> think that the level of testsuite development to which you have held yourself
> when doing development (such as your tests for various things in the new C
> parser) is an appropriate standard to impose on everyone else.
If the only issue were inadequate tests, not that other comments were
generally ignored, I'd just write and commit the tests and file bugs for
any that don't work!
I have two different standards of patch review. I apply the higher
standard - raising any possible problem with the contribution without
reference to whether it is something to block the contribution's inclusion
- when I'm *not* the person (in this case, Zack) who should ultimately
make the decision on whether the contribution can go in - as in such a
case that person can always decide that a particular issue is
insufficiently important. In this case, Zack had already provided in
advance a prioritisation of the issues I identified (bug 9449, comment
21). I don't generally state when commenting on a patch which standard of
review I am applying or the importance of particular comments; this,
together with applying the higher standard when I *won't* ultimately be
approving or rejecting the patch, sometimes seems to confuse people
reading my reviews.
There's broad discretion to the contributor as to exactly how they wish to
test a feature; for example, though personally I'd test the lists of UCNs
permitted in C and C++ by having tests for the UCNs at either end of every
range and one outside every range at each end (auto-generating this test,
not writing it by hand), I wouldn't insist that this is how this must be
tested if the tests suffice to show that different lists are accepted for
C and C++, that the corrections in C++03 relative to C++98 are present and
that UCNs for digits can't begin identifiers in C. Most of the points I
listed only need one or two tests - for example, the regression I list
below suffices to test that UCNs aren't lexed as such in identifiers in
C90.
The provided tests are a useful start for two of the points I list,
equivalence of different UCNs for same character (for which they suffice
except maybe for testing that a macro can't have two parameters whose
names are differently spelt versions of the same identifier) and of
different types of identifiers (though I noted that structure members,
labels and macro parameter names should also be covered). If they were
put in gcc.dg/debug then they'd also test that debug information
generation isn't broken by UCNs in identifiers (and that the assembler and
linker can handle the debug information generated). But, (a) there are
lots of miscellaneous points that should have individual tests (and I
listed those points in bug 9449 rather than expecting them to be guessed),
(b) in this case, writing testcases is *not* likely to be a significant
part of the time involved in implementing this feature; the vast bulk of
the time should, as Zack stated in bug 9449 comment 25, be that for the
due diligence audits Zack has explained the need for of everywhere
identifiers are used and especially where one sort of identifier (input,
output, internal, diagnostic) becomes another.
> Our development plan gives us one option. If two people with write privileges
> think the best course of action is to revert the patch, then we may revert the
> patch after 48 hours. Neil and Joseph have alread indicating they think this
> is best; I do, as well. But, this policy only applies to patches which
> introduce a regression. I am not aware of any regressions introduced by the
> patch, so until someone finds one we cannot start that 48-hour clock. I
> expect that we never contemplated a commit occuring in the face of such clear
> objections from appropriate maintainers.
Preprocess or compile the following with -std=c89.
#define a b(
#define b(x) q
int a\u00aa);
It should preprocess to
int q;
but with this patch in it preprocesses to
int a\U000000aa);
(which, in the context of preserving spelling, is wrong for C99 was well,
though if the preprocessor output only goes to the compiler it doesn't
matter). I mentioned this as a missing test in
<http://gcc.gnu.org/ml/gcc-patches/2005-03/msg01171.html> when the
previous version of this patch was posted, noting the test would be
similar to that for greedy lexing (which I provided in bug 9449 comment
34).
The tests in the patch also fail on Geoff's own regression tester, see
e.g. <http://gcc.gnu.org/ml/gcc-testresults/2005-03/msg00854.html>; I
don't know why.
Incomplete patches like this which break things and don't implement
required features such as warnings for identifiers not in NFKC can go on
branches - e.g. a new extended-identifiers-branch, or one of the Apple
branches - for all the features and tests to be added until they are ready
for mainline; it's not mainline-ready at present. Of course the branch
merge patch should itself be properly reviewed and compared against the
checklist to make sure it does everything necessary, not just committed to
mainline on a unilateral claim that it is ready.
--
Joseph S. Myers http://www.srcf.ucam.org/~jsm28/gcc/
jsm@polyomino.org.uk (personal mail)
joseph@codesourcery.com (CodeSourcery mail)
jsm28@gcc.gnu.org (Bugzilla assignments and CCs)