Re: [PATCH] c-family: Fix regression in location-overflow-test-1.c [PR97117]

2020-11-06 Thread Jeff Law via Gcc-patches


On 10/14/20 9:56 AM, Patrick Palka via Gcc-patches wrote:
> The r11-3266 patch that added macro support to -Wmisleading-indentation
> accidentally suppressed the column-tracking diagnostic in
> get_visual_column in some cases, e.g. in the location-overflow-test-1.c
> testcase.
>
> More generally, when all three tokens are on the same line and we've run
> out of locations with column info, then their location_t values will be
> equal, and we exit early from should_warn_for_misleading_indentation due
> to the new check
>
>   /* Give up if the loci are not all distinct.  */
>   if (guard_loc == body_loc || body_loc == next_stmt_loc)
> return false;
>
> before we ever call get_visual_column.
>
> [ This new check is needed to detect and give up on analyzing code
>   fragments where exactly two out of the three tokens come from the same
>   macro expansion, e.g.
>
> #define MACRO \
>   if (a)  \
> foo ();
>
> MACRO; bar ();
>
>   Here, guard_loc and body_loc will be equal and point to the macro
>   expansion point.  The heuristics the warning uses are not really valid
>   in scenarios like these.  ]
>
> In order to restore the column-tracking diagnostic, this patch moves the
> the diagnostic code out from get_visual_column to earlier in
> should_warn_for_misleading_indentation.  Moreover, it tests the three
> location_t values for a zero column all at once, which I suppose should
> make us issue the diagnostic more consistently.
>
> Tested on x86_64-pc-linux-gnu, does this look OK to commit?
>
> gcc/c-family/ChangeLog:
>
>   PR testsuite/97117
>   * c-indentation.c (get_visual_column): Remove location_t
>   parameter.  Move the column-tracking diagnostic code from here
>   to ...
>   (should_warn_for_misleading_indentation): ... here, before the
>   early exit for when the loci are not all distinct.  Don't pass a
>   location_t argument to get_visual_column.
>   (assert_get_visual_column_succeeds): Don't pass a location_t
>   argument to get_visual_column.
>   (assert_get_visual_column_fails): Likewise.

OK.


jeff




[PATCH] c-family: Fix regression in location-overflow-test-1.c [PR97117]

2020-10-14 Thread Patrick Palka via Gcc-patches
The r11-3266 patch that added macro support to -Wmisleading-indentation
accidentally suppressed the column-tracking diagnostic in
get_visual_column in some cases, e.g. in the location-overflow-test-1.c
testcase.

More generally, when all three tokens are on the same line and we've run
out of locations with column info, then their location_t values will be
equal, and we exit early from should_warn_for_misleading_indentation due
to the new check

  /* Give up if the loci are not all distinct.  */
  if (guard_loc == body_loc || body_loc == next_stmt_loc)
return false;

before we ever call get_visual_column.

[ This new check is needed to detect and give up on analyzing code
  fragments where exactly two out of the three tokens come from the same
  macro expansion, e.g.

#define MACRO \
  if (a)  \
foo ();

MACRO; bar ();

  Here, guard_loc and body_loc will be equal and point to the macro
  expansion point.  The heuristics the warning uses are not really valid
  in scenarios like these.  ]

In order to restore the column-tracking diagnostic, this patch moves the
the diagnostic code out from get_visual_column to earlier in
should_warn_for_misleading_indentation.  Moreover, it tests the three
location_t values for a zero column all at once, which I suppose should
make us issue the diagnostic more consistently.

Tested on x86_64-pc-linux-gnu, does this look OK to commit?

gcc/c-family/ChangeLog:

PR testsuite/97117
* c-indentation.c (get_visual_column): Remove location_t
parameter.  Move the column-tracking diagnostic code from here
to ...
(should_warn_for_misleading_indentation): ... here, before the
early exit for when the loci are not all distinct.  Don't pass a
location_t argument to get_visual_column.
(assert_get_visual_column_succeeds): Don't pass a location_t
argument to get_visual_column.
(assert_get_visual_column_fails): Likewise.
---
 gcc/c-family/c-indentation.c | 70 ++--
 1 file changed, 34 insertions(+), 36 deletions(-)

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 8b88a8adc7c..836a524f266 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -45,36 +45,11 @@ next_tab_stop (unsigned int vis_column, unsigned int 
tab_width)
on the line (up to or before EXPLOC).  */
 
 static bool
-get_visual_column (expanded_location exploc, location_t loc,
+get_visual_column (expanded_location exploc,
   unsigned int *out,
   unsigned int *first_nws,
   unsigned int tab_width)
 {
-  /* PR c++/68819: if the column number is zero, we presumably
- had a location_t > LINE_MAP_MAX_LOCATION_WITH_COLS, and so
- we have no column information.
- Act as if no conversion was possible, triggering the
- error-handling path in the caller.  */
-  if (!exploc.column)
-{
-  static bool issued_note = false;
-  if (!issued_note)
-   {
- /* Notify the user the first time this happens.  */
- issued_note = true;
- inform (loc,
- "%<-Wmisleading-indentation%> is disabled from this point"
- " onwards, since column-tracking was disabled due to"
- " the size of the code/headers");
- if (!flag_large_source_files)
-   inform (loc,
-   "adding %<-flarge-source-files%> will allow for more" 
-   " column-tracking support, at the expense of compilation"
-   " time and memory");
-   }
-  return false;
-}
-
   char_span line = location_get_source_line (exploc.file, exploc.line);
   if (!line)
 return false;
@@ -325,14 +300,37 @@ should_warn_for_misleading_indentation (const 
token_indent_info &guard_tinfo,
NULL);
 }
 
-  /* Give up if the loci are not all distinct.  */
-  if (guard_loc == body_loc || body_loc == next_stmt_loc)
-return false;
-
   expanded_location body_exploc = expand_location (body_loc);
   expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
   expanded_location guard_exploc = expand_location (guard_loc);
 
+  /* PR c++/68819: if the column number is zero, we presumably
+ had a location_t > LINE_MAP_MAX_LOCATION_WITH_COLS, and so
+ we have no column information.  */
+  if (!guard_exploc.column || !body_exploc.column || !next_stmt_exploc.column)
+{
+  static bool issued_note = false;
+  if (!issued_note)
+   {
+ /* Notify the user the first time this happens.  */
+ issued_note = true;
+ inform (guard_loc,
+ "%<-Wmisleading-indentation%> is disabled from this point"
+ " onwards, since column-tracking was disabled due to"
+ " the size of the code/headers");
+ if (!flag_large_source_files)
+   inform (guard_loc,
+