diff mbox

[pph] Add error for pre-parsing unguarded headers.

Message ID AANLkTi=imRuCZHEB4fRV75vEbaUteTBdmBWcp_7jV0L1@mail.gmail.com
State New
Headers show

Commit Message

Lawrence Crowl Feb. 24, 2011, 9:30 p.m. UTC
Add an error when pre-parsing a header that does not have a double
inclusion guard.

We do this by adding function cpp_main_missing_guard that returns
the name of the main source file when it does not have a guard, and
checking that condition before writing the PPH file.

Index: gcc/testsuite/ChangeLog.pph

2011-02-24  Lawrence Crowl <crowl@google.com>

	* g++.dg/pph/d1unguarded.h: Add test for non-null unguarded header.
        * g++.dg/pph/d2null.h: Unguarded header error now expected.

Index: gcc/cp/ChangeLog.pph

2011-02-24  Lawrence Crowl <crowl@google.com>

        * cp/pph.c (pph_finish): Check for missing preprocessor guard
        for pre-parsed headers.

Index: libcpp/ChangeLog.pph

2011-02-24  Lawrence Crowl  <crowl@google.com>

	* include/cpplib.h (cpp_main_missing_guard): Add declaration.
	* files.c (cpp_main_missing_guard): Add definition.
	(main_missing_guard): Add to support cpp_main_missing_guard.

Comments

Diego Novillo Feb. 24, 2011, 10:44 p.m. UTC | #1
On 11-02-24 04:30 PM, Lawrence Crowl wrote:

> --- 4094,4106 ----
>   pph_finish (void)
>   {
>     if (pph_out_file != NULL)
> !     {
> !       const char* offending_file = cpp_main_missing_guard (parse_in);

s/char* /char */


> +   /* Skip directories.  */
> +   if (entry->start_dir != NULL)
> +     {
> +       _cpp_file *file = entry->u.file;
> +
> +       /* FIXME pph: It's not clear what effect once_only should have.  */
> +       if (!file->once_only && file->cmacro == NULL
> +           && file->stack_count == 1 && file->main_file)

Line the predicates vertically.

Shouldn't #pragma once make the file pph-friendly?



> +
> + /* Return the path name of the main file iff it does not have a
> +    multiple include guard. */

Doesn't this return the name of *a* file that is not double-guarded?


Diego.
Lawrence Crowl Feb. 24, 2011, 11:27 p.m. UTC | #2
On 2/24/11, Diego Novillo <dnovillo@google.com> wrote:
> On 11-02-24 04:30 PM, Lawrence Crowl wrote:
>> --- 4094,4106 ----
>>   pph_finish (void)
>>   {
>>     if (pph_out_file != NULL)
>> !     {
>> !       const char* offending_file = cpp_main_missing_guard (parse_in);
>
> s/char* /char */

Fixed.

You are in a maze of twisty coding conventions, none alike.

>> +   /* Skip directories.  */
>> +   if (entry->start_dir != NULL)
>> +     {
>> +       _cpp_file *file = entry->u.file;
>> +
>> +       /* FIXME pph: It's not clear what effect once_only should have.
>> */
>> +       if (!file->once_only && file->cmacro == NULL
>> +           && file->stack_count == 1 && file->main_file)
>
> Line the predicates vertically.

Fixed.

> Shouldn't #pragma once make the file pph-friendly?

Technically, yes.  It was unclear to me whether that was something
we wanted to support.  The current code is conservative, in that
support can be added without incompatibilities.

>> +
>> + /* Return the path name of the main file iff it does not have a
>> +    multiple include guard. */
>
> Doesn't this return the name of *a* file that is not double-guarded?

The test for file->main_file ensures that we return at most the
main file.  (Unless somehow we get two main files.)
Diego Novillo Feb. 24, 2011, 11:37 p.m. UTC | #3
On Thu, Feb 24, 2011 at 18:27, Lawrence Crowl <crowl@google.com> wrote:

> You are in a maze of twisty coding conventions, none alike.

:)

> Technically, yes.  It was unclear to me whether that was something
> we wanted to support.  The current code is conservative, in that
> support can be added without incompatibilities.

Sounds good.

> The test for file->main_file ensures that we return at most the
> main file.  (Unless somehow we get two main files.)

Ah, yeah, I missed that.

Patch is OK.  Thanks.


Diego.
diff mbox

Patch

Index: gcc/testsuite/g++.dg/pph/d1unguarded.h
===================================================================
*** gcc/testsuite/g++.dg/pph/d1unguarded.h	(revision 0)
--- gcc/testsuite/g++.dg/pph/d1unguarded.h	(revision 0)
***************
*** 0 ****
--- 1 ----
+ extern int x; // { dg-error "header lacks guard for PPH" }
Index: gcc/testsuite/g++.dg/pph/d2null.h
===================================================================
*** gcc/testsuite/g++.dg/pph/d2null.h	(revision 170280)
--- gcc/testsuite/g++.dg/pph/d2null.h	(working copy)
***************
*** 1 ****
! // { dg-error "header lacks guard for PPH" "" { xfail *-*-* } }
--- 1 ----
! // { dg-error "header lacks guard for PPH" }
Index: gcc/cp/pph.c
===================================================================
*** gcc/cp/pph.c	(revision 170280)
--- gcc/cp/pph.c	(working copy)
*************** void
*** 4094,4100 ****
  pph_finish (void)
  {
    if (pph_out_file != NULL)
!     write_pph_output ();
  
    if (flag_pph_stats)
      pph_print_stats ();
--- 4094,4106 ----
  pph_finish (void)
  {
    if (pph_out_file != NULL)
!     {
!       const char* offending_file = cpp_main_missing_guard (parse_in);
!       if (offending_file == NULL)
!         write_pph_output ();
!       else
!         error ("header lacks guard for PPH: %s", offending_file);
!     }
  
    if (flag_pph_stats)
      pph_print_stats ();
Index: libcpp/include/cpplib.h
===================================================================
*** libcpp/include/cpplib.h	(revision 170279)
--- libcpp/include/cpplib.h	(working copy)
*************** extern void cpp_clear_file_cache (cpp_re
*** 1013,1018 ****
--- 1013,1021 ----
  extern cpp_offset cpp_get_pos (cpp_buffer *);
  extern void cpp_set_pos (cpp_buffer *, cpp_offset);
  extern void cpp_return_at_eof (cpp_buffer *, bool);
+ /* Return the path name of the main file iff it does not have a
+    multiple include guard. */
+ extern const char *cpp_main_missing_guard (cpp_reader *pfile);
  
  /* In pch.c */
  struct save_macro_data;
Index: libcpp/files.c
===================================================================
*** libcpp/files.c	(revision 170279)
--- libcpp/files.c	(working copy)
*************** _cpp_report_missing_guards (cpp_reader *
*** 1320,1325 ****
--- 1320,1362 ----
      }
  }
  
+ 
+ /* Callback function for cpp_main_missing_guard with htab_traverse.  */
+ static int
+ main_missing_guard (void **slot, void *d)
+ {
+   struct file_hash_entry *entry = (struct file_hash_entry *) *slot;
+   const char **dropbox = (const char **) d;
+ 
+   /* Skip directories.  */
+   if (entry->start_dir != NULL)
+     {
+       _cpp_file *file = entry->u.file;
+ 
+       /* FIXME pph: It's not clear what effect once_only should have.  */
+       if (!file->once_only && file->cmacro == NULL
+           && file->stack_count == 1 && file->main_file)
+         {
+           *dropbox = file->path;
+           return 0; /* Stop traversing the hash table.  */
+         }
+     }
+ 
+   /* Keep traversing the hash table.  */
+   return 1;
+ }
+ 
+ /* Return the path name of the main file iff it does not have a 
+    multiple include guard. */
+ const char *
+ cpp_main_missing_guard (cpp_reader *pfile)
+ {
+   const char *offending_file = false;
+   htab_traverse (pfile->file_hash, main_missing_guard, &offending_file);
+   return offending_file;
+ }
+ 
+ 
  /* Locate HEADER, and determine whether it is newer than the current
     file.  If it cannot be located or dated, return -1, if it is
     newer, return 1, otherwise 0.  */