Message ID | 20160331141438.GD13362@redhat.com |
---|---|
State | New |
Headers | show |
On 03/31/2016 08:14 AM, Marek Polacek wrote: > On Wed, Mar 30, 2016 at 11:51:26PM +0100, Manuel López-Ibáñez wrote: >> On 30 March 2016 at 23:42, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: >>> On 30/03/16 17:14, Marek Polacek wrote: >>>> >>>> This test ICEs since the addition of the assert in pp_string which ensures >>>> that >>>> we aren't trying to print an empty string. But that's what happens here, >>>> the >>>> location is actually UNKNOWN_LOCATION, so LOCATION_FILE on that yields >>>> null. >>>> Fixed byt not trying to print the filename of UNKNOWN_LOCATION. >> >>> Even if we accept the broken location for now (adding some FIXME to the code >>> would help the next person to realise this is not normal), if >>> LOCATION_FILE() is NULL, we should print "progname" like >>> diagnostic_build_prefix() does. Moreover, the filename string should be >>> built with file_name_as_prefix() to get correct coloring. >> >> Even better: Use "f ? f : progname" in file_name_as_prefix() and >> simplify the code to: > > It seems wrong to me to just add that. I wonder if the function shouldn't > use diagnostic_get_location_text instead which also handles "<built-in>" etc. > In any case I'd rather not touch the diagnostic stuff in the scope of this > patch (which wouldn't probably be suitable for stage4 anyway). > >> /* FIXME: Somehow we may get UNKNOWN_LOCATION here: See >> g++.dg/cpp0x/constexpr-70449.C */ >> const char * prefix = file_name_as_prefix (context, >> LOCATION_FILE (location)); >> pp_verbatim (context->printer, >> TREE_CODE (p->decl) == TREE_LIST >> ? _("%s: In substitution of %qS:\n") >> : _("%s: In instantiation of %q#D:\n"), >> prefix, p->decl); >> free (prefix); >> >> Fixes the ICE, adds colors, mentions the broken location and does not >> add extra strings. > > It fixes the ICE, but I don't see any more colors than with my patch. > Moreover your suggestion would print > cc1plus: : In instantiation of ‘constexpr int f() [with int I = 0]’: > , not sure how's that better than simply saying > In instantiation of ‘constexpr int f() [with int I = 0]’: > I.e. adding "cc1plus" to the output doesn't seem like an improvement. > > Anyway, here's a patch which uses file_name_as_prefix. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2016-03-31 Marek Polacek <polacek@redhat.com> > > PR c++/70449 > * error.c (print_instantiation_full_context): Build prefix by using > file_name_as_prefix. > > * g++.dg/cpp0x/constexpr-70449.C: New test. I'd prefer to dig into why we don't have a good location here before approving this patch. Depending on what's found, I might approve the patch as-is or request the root cause be fixed. Obviously if you get conflicting feedback from Jason, then ignore mine. jeff
diff --git gcc/cp/error.c gcc/cp/error.c index aa5fd41..14b0205 100644 --- gcc/cp/error.c +++ gcc/cp/error.c @@ -3312,12 +3312,15 @@ print_instantiation_full_context (diagnostic_context *context) if (p) { + char *prefix = file_name_as_prefix (context, LOCATION_FILE (location) + ? LOCATION_FILE (location) + : progname); pp_verbatim (context->printer, TREE_CODE (p->decl) == TREE_LIST - ? _("%s: In substitution of %qS:\n") - : _("%s: In instantiation of %q#D:\n"), - LOCATION_FILE (location), - p->decl); + ? _("%sIn substitution of %qS:\n") + : _("%sIn instantiation of %q#D:\n"), + prefix, p->decl); + free (prefix); location = p->locus; p = p->next; diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C index e69de29..bc5dd71 100644 --- gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C +++ gcc/testsuite/g++.dg/cpp0x/constexpr-70449.C @@ -0,0 +1,12 @@ +// PR c++/70449 +// { dg-do compile { target c++11 } } + +template <int> +constexpr +int f (void) +{ + enum E { a = f<0> () }; + return 0; +} + +// { dg-error "body of constexpr function" "" { target { ! c++14 } } 0 }