This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
proposal to add -Wheader-guard option
- From: Prathamesh Kulkarni <bilbotheelffriend at gmail dot com>
- To: gcc-patches at gcc dot gnu dot org
- Date: Tue, 4 Feb 2014 15:06:32 +0530
- Subject: proposal to add -Wheader-guard option
- Authentication-results: sourceware.org; auth=none
Hi, I was wondering if it's a good idea to add -Wheader-guard option
that warns on mismatches between #ifndef and #define lines
in header guard, similar to -Wheader-guard in clang-3.4 ?
(http://llvm.org/releases/3.4/tools/clang/docs/ReleaseNotes.html)
I have implemented patch for -Wheader-guard (please find it attached).
Consider a file having the following format:
#ifndef cmacro (or #if !defined(cmacro) )
#define dmacro
// rest of the stuff
#endif
The warning is triggered if the edit distance
(http://en.wikipedia.org/wiki/Levenshtein_distance), between cmacro
and dmacro
is <= max (len(cmacro), len(dmacro)) / 2
If the edit distance is more than half, I assume that cmacro
and dmacro are "very different", and the intent
was probably not to define header guard (This is what clang does too).
Example:
#ifndef FOO_H
#define _FOO_H
#endif
foo.h:1:0: warning: FOO_H used as header guard followed by #define of
different macro [-Wheader-guard]
#ifndef FOO_H
^
foo.h:2:0: note: FOO_H is defined here, did you mean _FOO_H ?
#define _FOO_H
^
Warning is not triggered in the following cases:
1] The edit distance between #ifndef (or #!defined) macro
and #define macro is > half of maximum length between two macros
Example:
#ifndef FOO
#define BAR
#endif
2] #ifndef and #define are not on consecutive lines (blank lines/ comment lines
are not ignored).
Example:
#ifndef cmacro
/* hello world */
extern int x;
#define dmacro
#endif
3] dmacro gets undefined
Example:
#ifndef cmacro
#define dmacro
#undef dmacro
#endif
However the following warning gets generated during the build:
../../src/libcpp/directives.c: In function 'void _cpp_pop_buffer(cpp_reader*)':
../../src/libcpp/directives.c:
2720:59: warning: 'inc_type' may be used
uninitialized in this function [-Wmaybe-uninitialized]
_cpp_pop_file_buffer (pfile, inc, to_free, inc_type);
^
_cpp_pop_buffer(): http://pastebin.com/aLYLnXJa
I have defined inc_type only if inc is not null (ie buffer is file
buffer) in 1st if()
and used it (passed it to _cpp_pop_file_buffer() ), only if inc is not null
in 2nd if(). I guess this warning could be considered harmless ?
How should I should rewrite it to avoid the warning ?
Thanks and Regards,
Prathamesh
Index: libcpp/directives.c
===================================================================
--- libcpp/directives.c (revision 207451)
+++ libcpp/directives.c (working copy)
@@ -565,6 +565,27 @@ lex_macro_node (cpp_reader *pfile, bool
return NULL;
}
+// return true if top of if_stack is cmacro
+static bool
+cmacro_ifs_top_p(cpp_reader *pfile)
+{
+ struct if_stack *ifs = pfile->buffer->if_stack;
+ return ifs && (ifs->next == NULL) && (ifs->mi_cmacro != NULL);
+}
+
+static linenum_type
+linediff (struct line_maps *maps, source_location loc1, source_location loc2)
+{
+ linenum_type temp;
+
+ if (loc1 < loc2)
+ temp = loc1, loc1 = loc2, loc2 = temp;
+
+ const struct line_map *m1 = linemap_lookup (maps, loc1);
+ const struct line_map *m2 = linemap_lookup (maps, loc2);
+ return SOURCE_LINE (m1, loc1) - SOURCE_LINE (m2, loc2);
+}
+
/* Process a #define directive. Most work is done in macro.c. */
static void
do_define (cpp_reader *pfile)
@@ -586,6 +607,15 @@ do_define (cpp_reader *pfile)
pfile->cb.define (pfile, pfile->directive_line, node);
node->flags &= ~NODE_USED;
+
+ // possibly #define following #ifndef in the include guard
+ if (pfile->buffer->dmacro == NULL && cmacro_ifs_top_p (pfile)
+ && linediff (pfile->line_table, pfile->directive_line, pfile->buffer->if_stack->line) == 1)
+ {
+ pfile->buffer->dmacro = node;
+ pfile->buffer->cmacro_loc = pfile->buffer->if_stack->line;
+ pfile->buffer->dmacro_loc = pfile->directive_line;
+ }
}
}
@@ -2527,6 +2557,104 @@ cpp_get_deps (cpp_reader *pfile)
return pfile->deps;
}
+static inline unsigned
+ed_min3 (unsigned a, unsigned b, unsigned c)
+{
+ return (a < b) ? (a < c) ? a : c
+ : (b < c) ? b : c;
+}
+
+/*
+ * Levenshtein distance: http://en.wikipedia.org/wiki/Levenshtein_distance
+ * The algorithm is implemented using dynamic programming.
+ * Instead of the matrix[s_len][t_len], we only need storage
+ * for one row with length = min(s_len, t_len), since we look only at one row and it's previous row
+ * at a time (and previous row's contents are replaced in place).
+ * hstr is string laid along the row of matrix and vstr is laid along column
+ * hstr is string with lesser length between s and t.
+ */
+
+static unsigned
+edit_dist (const unsigned char *s, const unsigned char *t,
+ unsigned s_len, unsigned t_len)
+{
+ unsigned *row;
+ unsigned n_rows, n_cols, i, j, prev, temp, dist;
+ const unsigned char *hstr, *vstr;
+
+ if (s_len < t_len)
+ {
+ hstr = s;
+ vstr = t;
+ n_cols = s_len + 1;
+ n_rows = t_len + 1;
+ }
+ else
+ {
+ hstr = t;
+ vstr = s;
+ n_cols = t_len + 1;
+ n_rows = s_len + 1;
+ }
+
+ row = (unsigned *) xmalloc (sizeof (*row) * n_cols);
+ for (j = 0; j < n_cols; j++)
+ row[j] = j;
+
+ for (i = 1; i < n_rows; i++)
+ {
+ row[0] = i;
+ for (j = 1, prev = i - 1; j < n_cols; j++)
+ {
+ temp = row[j];
+ row[j] = ed_min3 (row[j - 1] + 1, row[j] + 1, prev + (vstr[i-1] != hstr[j-1]));
+ prev = temp;
+ }
+ }
+
+ dist = row[n_cols - 1];
+ free (row);
+ return dist;
+}
+
+
+static void
+warn_header_guard (cpp_reader *pfile)
+{
+ const cpp_hashnode *cmacro = pfile->mi_cmacro;
+ const cpp_hashnode *dmacro = pfile->buffer->dmacro;
+
+ /*
+ * _cpp_file_cmacro_defined_p (), takes care that we are not warning again
+ * for the same file (for example if it's included twice)
+ * if cmacro is defined, or dmacro gets undefined return
+ */
+
+ if (_cpp_file_cmacro_defined_p (pfile->buffer->file) || cmacro == NULL || cmacro->type != NT_VOID
+ || dmacro == NULL || dmacro->type != NT_MACRO)
+ return;
+
+ const unsigned char *cmacro_name = NODE_NAME (cmacro);
+ unsigned cmacro_len = NODE_LEN (cmacro);
+
+ const unsigned char *dmacro_name = NODE_NAME (dmacro);
+ unsigned dmacro_len = NODE_LEN (dmacro);
+
+ unsigned maxhalflen = (cmacro_len > dmacro_len ? cmacro_len : dmacro_len) >> 1;
+ unsigned ed = edit_dist (cmacro_name, dmacro_name, cmacro_len, dmacro_len);
+
+ if (ed <= maxhalflen)
+ {
+ cpp_warning_with_line (pfile, CPP_W_HEADER_GUARD, pfile->buffer->cmacro_loc, 0,
+ "%s used as header guard followed by #define of different macro",
+ cmacro_name);
+
+ cpp_error_with_line (pfile, CPP_DL_NOTE, pfile->buffer->dmacro_loc, 0,
+ "%s is defined here, did you mean %s ?",
+ cmacro_name, dmacro_name);
+ }
+}
+
/* Push a new buffer on the buffer stack. Returns the new buffer; it
doesn't fail. It does not generate a file change call back; that
is the responsibility of the caller. */
@@ -2559,6 +2687,7 @@ _cpp_pop_buffer (cpp_reader *pfile)
struct _cpp_file *inc = buffer->file;
struct if_stack *ifs;
const unsigned char *to_free;
+ enum include_type inc_type;
/* Walk back up the conditional stack till we reach its level at
entry to this file, issuing error messages. */
@@ -2566,6 +2695,13 @@ _cpp_pop_buffer (cpp_reader *pfile)
cpp_error_with_line (pfile, CPP_DL_ERROR, ifs->line, 0,
"unterminated #%s", dtable[ifs->type].name);
+ if (inc)
+ {
+ inc_type = buffer->inc_type;
+ if (CPP_WHEADER_GUARD (pfile) && pfile->mi_valid)
+ warn_header_guard (pfile);
+ }
+
/* In case of a missing #endif. */
pfile->state.skipping = 0;
@@ -2581,7 +2717,7 @@ _cpp_pop_buffer (cpp_reader *pfile)
if (inc)
{
- _cpp_pop_file_buffer (pfile, inc, to_free);
+ _cpp_pop_file_buffer (pfile, inc, to_free, inc_type);
_cpp_do_file_change (pfile, LC_LEAVE, 0, 0, 0);
}
Index: libcpp/files.c
===================================================================
--- libcpp/files.c (revision 207451)
+++ libcpp/files.c (working copy)
@@ -198,6 +198,12 @@ static int pchf_save_compare (const void
static int pchf_compare (const void *d_p, const void *e_p);
static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
+bool
+_cpp_file_cmacro_defined_p (_cpp_file *file)
+{
+ return file->cmacro != NULL;
+}
+
/* Given a filename in FILE->PATH, with the empty string interpreted
as <stdin>, open it.
@@ -859,6 +865,14 @@ should_stack_file (cpp_reader *pfile, _c
return f == NULL;
}
+/* Initialize controlling macro state */
+static inline void
+mi_init (cpp_reader *pfile)
+{
+ pfile->mi_valid = true;
+ pfile->mi_cmacro = 0;
+}
+
/* Place the file referenced by FILE into a new buffer on the buffer
stack if possible. IMPORT is true if this stacking attempt is
because of a #import directive. Returns true if a buffer is
@@ -895,10 +909,10 @@ _cpp_stack_file (cpp_reader *pfile, _cpp
buffer->file = file;
buffer->sysp = sysp;
buffer->to_free = file->buffer_start;
+ buffer->dmacro = 0;
/* Initialize controlling macro state. */
- pfile->mi_valid = true;
- pfile->mi_cmacro = 0;
+ mi_init (pfile);
/* Generate the call back. */
_cpp_do_file_change (pfile, LC_ENTER, file->path, 1, sysp);
@@ -1012,7 +1026,8 @@ _cpp_stack_include (cpp_reader *pfile, c
/* _cpp_stack_file didn't stack the file, so let's rollback the
compensation dance we performed above. */
pfile->line_table->highest_location++;
-
+ else
+ pfile->buffer->inc_type = type;
return stacked;
}
@@ -1445,7 +1460,7 @@ cpp_push_default_include (cpp_reader *pf
input stack. */
void
_cpp_pop_file_buffer (cpp_reader *pfile, _cpp_file *file,
- const unsigned char *to_free)
+ const unsigned char *to_free, enum include_type inc_type)
{
/* Record the inclusion-preventing macro, which could be NULL
meaning no controlling macro. */
@@ -1453,6 +1468,10 @@ _cpp_pop_file_buffer (cpp_reader *pfile,
file->cmacro = pfile->mi_cmacro;
/* Invalidate control macros in the #including file. */
+
+ if (inc_type == IT_DEFAULT || inc_type == IT_CMDLINE)
+ mi_init (pfile);
+ else
pfile->mi_valid = false;
if (to_free)
Index: libcpp/include/cpplib.h
===================================================================
--- libcpp/include/cpplib.h (revision 207451)
+++ libcpp/include/cpplib.h (working copy)
@@ -354,6 +354,9 @@ struct cpp_options
traditional C. */
unsigned char cpp_warn_traditional;
+ /* Nonzero means warn about header guard */
+ unsigned char cpp_warn_header_guard;
+
/* Nonzero means warn about long long numeric constants. */
unsigned char cpp_warn_long_long;
@@ -933,7 +936,8 @@ enum {
CPP_W_INVALID_PCH,
CPP_W_WARNING_DIRECTIVE,
CPP_W_LITERAL_SUFFIX,
- CPP_W_DATE_TIME
+ CPP_W_DATE_TIME,
+ CPP_W_HEADER_GUARD
};
/* Output a diagnostic of some kind. */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h (revision 207451)
+++ libcpp/internal.h (working copy)
@@ -348,6 +348,11 @@ struct cpp_buffer
needs to be extern "C" protected in C++, and zero otherwise. */
unsigned char sysp;
+ enum include_type inc_type;
+ const cpp_hashnode *dmacro;
+ source_location cmacro_loc;
+ source_location dmacro_loc;
+
/* The directory of the this buffer's file. Its NAME member is not
allocated, so we don't need to worry about freeing it. */
struct cpp_dir dir;
@@ -597,6 +602,7 @@ cpp_in_system_header (cpp_reader *pfile)
}
#define CPP_PEDANTIC(PF) CPP_OPTION (PF, cpp_pedantic)
#define CPP_WTRADITIONAL(PF) CPP_OPTION (PF, cpp_warn_traditional)
+#define CPP_WHEADER_GUARD(PF) CPP_OPTION (PF, cpp_warn_header_guard)
static inline int cpp_in_primary_file (cpp_reader *);
static inline int
@@ -640,11 +646,12 @@ extern void _cpp_report_missing_guards (
extern void _cpp_init_files (cpp_reader *);
extern void _cpp_cleanup_files (cpp_reader *);
extern void _cpp_pop_file_buffer (cpp_reader *, struct _cpp_file *,
- const unsigned char *);
+ const unsigned char *, enum include_type);
extern bool _cpp_save_file_entries (cpp_reader *pfile, FILE *f);
extern bool _cpp_read_file_entries (cpp_reader *, FILE *);
extern const char *_cpp_get_file_name (_cpp_file *);
extern struct stat *_cpp_get_file_stat (_cpp_file *);
+extern bool _cpp_file_cmacro_defined_p (_cpp_file *);
/* In expr.c */
extern bool _cpp_parse_expr (cpp_reader *, bool);
Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c (revision 207451)
+++ libcpp/line-map.c (working copy)
@@ -35,6 +35,7 @@ static source_location linemap_macro_map
(const struct line_map*, source_location);
static source_location linemap_macro_map_loc_unwind_toward_spelling
(const struct line_map*, source_location);
+ char *p;
static source_location linemap_macro_map_loc_to_exp_point
(const struct line_map*, source_location);
static source_location linemap_macro_loc_to_spelling_point
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c (revision 207451)
+++ gcc/c-family/c-common.c (working copy)
@@ -9577,6 +9577,7 @@ static const struct reason_option_codes_
{CPP_W_WARNING_DIRECTIVE, OPT_Wcpp},
{CPP_W_LITERAL_SUFFIX, OPT_Wliteral_suffix},
{CPP_W_DATE_TIME, OPT_Wdate_time},
+ {CPP_W_HEADER_GUARD, OPT_Wheader_guard},
{CPP_W_NONE, 0}
};
Index: gcc/c-family/c-opts.c
===================================================================
--- gcc/c-family/c-opts.c (revision 207451)
+++ gcc/c-family/c-opts.c (working copy)
@@ -430,6 +430,10 @@ c_common_handle_option (size_t scode, co
cpp_opts->cpp_warn_traditional = value;
break;
+ case OPT_Wheader_guard:
+ cpp_opts->cpp_warn_header_guard = value;
+ break;
+
case OPT_Wtrigraphs:
cpp_opts->warn_trigraphs = value;
break;
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt (revision 207451)
+++ gcc/c-family/c.opt (working copy)
@@ -736,6 +736,10 @@ Wtraditional
C ObjC Var(warn_traditional) Warning
Warn about features not present in traditional C
+Wheader-guard
+C ObjC C++ ObjC++ Warning
+Warn of header guard
+
Wtraditional-conversion
C ObjC Var(warn_traditional_conversion) Warning
Warn of prototypes causing type conversions different from what would happen in the absence of prototype