[patch, fortran] PR18428: use libcpp for preprocessing (review of C/C++ and fortran maintainers needed)

[FX]

Hi,

This is a review of the Fortran parts of the patch. It still needs  
review by C maintainer (I think the renaming in cp/Makefile.in falls  
under the trivial rule). The Fortran parts are OK provided you answer  
the questions below and:
   -- provide a patch for the news page (http://gcc.gnu.org/gcc-4.4/ 
changes.html),
   -- rewrite the doc for preprocessing (http://gcc.gnu.org/ 
onlinedocs/gfortran/Preprocessing-and-conditional-compilation.html);  
I suggest you add a link to CPP options there.

Thanks for the overall very well written and commented patch, it was  
a real pleasure to review.



> @@ -354,6 +357,9 @@ gfc_post_options (const char **pfilename
>    if (gfc_option.flag_all_intrinsics)
>      gfc_option.warn_nonstd_intrinsics = 0;
>
> +  gfc_cpp_post_options ();
> +
> +/* FIXME: return gfc_cpp_preprocess_only (); */
>    return false;
>  }

I don't understand this FIXME: why should we do that? And why isn't  
it done here?


> +d
> +Fortran Joined
> +-d[DIMN]	Dump details about macro names and definitions during  
> preprocessing

Aren't you missing the 'U' in that list?

> +/* Objects defined in libcpp:  */
> +static cpp_options *cpp_option = NULL;
> +static cpp_reader *cpp_in = NULL;

If they're defined in libcpp, why are they static here? And a grep in  
libcpp/ doesn't turn up anything with that name. Maybe you mean they  
contain libcpp objects, but another wording would be clearer.

> +  /* Initialize CPP built-ins; '1' corresponds to 'flag_hosted'
> +     in C, defines __STDC_HOSTED__?!  */
> +  cpp_init_builtins (cpp_in, 1);

I don't think we want to define __STDC_HOSTED__. On the other hand,  
does your patch still define the CPU, OS and OBJFMT builtins? (macros  
TARGET_CPU_CPP_BUILTINS, TARGET_OS_CPP_BUILTINS and  
TARGET_OBJFMT_CPP_BUILTINS) I think we should. Other macros that we  
used to emit and I think we will now miss include: __VERSION__, _LP64  
and __LP64__, __OPTIMIZE_SIZE__, __OPTIMIZE__, __FAST_MATH__,  
__SUPPORT_SNAN__, __FINITE_MATH_ONLY__, __pic__, __PIC__, __pie__,  
__PIE__, and the __SIZEOF_*__ macros (which are I think potentially  
useful).


> +    case OPT_C:
> +      gfc_cpp_option.discard_comments = 0;
> +      break;
> +
> +    case OPT_CC:
> +      gfc_cpp_option.discard_comments = 0;
> +      gfc_cpp_option.discard_comments_in_macro_exp = 0;
> +      break;

As a note, I was amazed that we support removal of C-style comments  
in preprocessor macros. I've checked that loads of other compilers  
also do, because it sounds weird, but such is life.

> +  if (gfc_cpp_option.working_directory
> +      && gfc_cpp_option.preprocess_only && ! 
> gfc_cpp_option.no_line_commands)
> +    pp_dir_change (cpp_in, get_src_pwd ());

I don't understand why it only happens with  
gfc_cpp_option.preprocess_only: what happens to debugging  
information? The documentation says that "This option is implicitly  
enabled if debugging information is enabled", is this also the case  
here?


> +/* static void print_lines_directives_only (int, const void *,  
> size_t); */

I guess this is a leftover from a previous version.

> +/* static void
> +print_lines_directives_only (int lines, const void *buf, size_t size)
> +{
> +  print.src_line += lines;
> +  fwrite (buf, 1, size, print.outf);
> +} */

Same here.


> +  /* Supply enough spaces to put this token in its original column,
> +     one space per column greater than 2, since scan_translation_unit
> +     will provide a space if PREV_WHITE.  Don't bother trying to
> +     reconstruct tabs; we can't get it right in general, and nothing
> +     ought to care.  Some things do care; the fault lies with  
> them.  */

Can you expand a bit more here? What kind of thing will change or be  
broken exactly?

> +/* Callback called when -fworking-director and -E to emit working
> +   directory in cpp output file.  */
> +
> +void
> +pp_dir_change (cpp_reader *pfile ATTRIBUTE_UNUSED, const char *dir)

Same comment here as earlier: why only when -E is used? Doesn't that  
break debug information?


FX

-- 
François-Xavier Coudert
http://www.homepages.ucl.ac.uk/~uccafco/