Re: [PATCH v2] Ability to remap file names in __FILE__, etc (PR other/70268)

[Martin Sebor <msebor at gmail dot com>]

On 12/07/2017 05:29 AM, Boris Kolpackov wrote:
> Thanks for the review. Second revision of the patch attached (also
> rebased on the current trunk). Issues that are not commented on
> below have been resolved as suggested.
>
>
> David Malcolm <dmalcolm@redhat.com> writes:
>
> > To my naive eyes this seems like a useful addition, but I'm hoping
> > someone with more knowledge of the standards around the preprocessor
> > can comment.
>
> This would definitely be a non-standard extension to any preprocessor
> standard there might be. Since it's not enabled by default I don't see
> any issues though.

FWIW, I agree, though the final word is of course Joseph and
Marek's as the C front end maintainers.

> > > +      error ("invalid argument %qs to %s", arg, opt);
> >
> > I think both of these should be %qs, so that the option is quoted (the
> > old code in final.c didn't do that, but I think it should have).
>
> I personally disagree since an option like -ffile-prefix-map is not
> easy to confuse with a language word. But I defer to you judgment.

We have been making an effort to add quoting around options
in diagnostics.  There are still many unquoted options left
but the (so far only informally agreed upon, AFAIK) goal is
to eventually quote them all.

I think adding options to the list in the Quoting section on
the DiagnosticsGuidelines Wiki would be appropriate.  Do you
agree, David?

> > > diff --git a/gcc/testsuite/c-c++-common/cpp/ffile-prefix-map.c
> > > b/gcc/testsuite/c-c++-common/cpp/ffile-prefix-map.c
> > > new file mode 100644
> > > index 00000000000..cf14de84a0d
> > > --- /dev/null
> > > +++ b/gcc/testsuite/c-c++-common/cpp/ffile-prefix-map.c
> > > @@ -0,0 +1,5 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-ffile-prefix-map==FILE-PREFIX" } */
> >
> > What's up with this "=="? (as opposed to "=").
>
> Since I cannot predict the actual path, I am remapping empty prefix
> to FILE-PREFIX which effectively adds FILE-PREFIX to any path.

IIUC, it's the same as "-ffile-prefix-map=/=/FILE-PREFIX", correct?
That seems useful to me, but I'm not sure it's as useful as it
could or might need to be in some cases.  E.g., if sources for
nightly builds are downloaded into a temporary directory with some
random name, this prepends a known prefix to the directory but it
doesn't make the whole path-name determinate.  Do you think it
would it be useful to add support for globbing, as a separate
enhancement?)

> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1175,7 +1175,11 @@ Common RejectNegative Joined Var(common_deferred_options) Defer
>
>  fdebug-prefix-map=
>  Common Joined RejectNegative Var(common_deferred_options) Defer
> -Map one directory name to another in debug information.
> +-fdebug-prefix-map=<old>=<new> Map one directory name to another in debug information.
> +
> +ffile-prefix-map=
> +Common Joined RejectNegative Var(common_deferred_options) Defer

AFAIK, RejectNegative applies to options that take integer arguments.
This one takes a string so it probably doesn't belong here.

> --- a/gcc/doc/cppopts.texi
> +++ b/gcc/doc/cppopts.texi
> @@ -287,6 +287,16 @@ When this option is given no argument, the default parameter value is
>
>  Note that @code{-ftrack-macro-expansion=2} is activated by default.
>
> +@item -fmacro-prefix-map=@var{old}=@var{new}
> +@opindex fmacro-prefix-map
> +When preprocessing files in directory @file{@var{old}}, expand the
> +@code{__FILE__} and @code{__BASE_FILE__} macros as in @file{@var{new}}
> +instead.

This could be just me, but to my eyes this sentence can be read
one of two ways: (1) files residing in directory old, or (2)
the current working directory is old while preprocessing some
files.  To make it 100% clear which is meant, I would find it
more accurate if it were phrased like this instead:

  When preprocessing files residing in directory @file{@var{old}},
  expand the @code{__FILE__} and @code{__BASE_FILE__} macros as if
  the files resided in directory @file{@var{new}} instead.

Ditto for the other options.  (I assume you chose the wording to
be consistent with the existing -fdebug-prefix-map=old=new option.
I'd find the documentation for that option clearer if it were
reworded as well.)

Martin