| Message ID | ebd05e3e-6cbc-08ce-8b61-e1dde6475825@iki.fi |
|---|---|
| State | New |
| Headers | show |
Hi! A few review comments: On Wed, 7 Sep 2016 20:19:20 +0300, Andris Pavenis <andris.pavenis@iki.fi> wrote: > This patch fixes handling header.gcc in subdirectories when command line option -remap has been > used. (I have not yet looked up what that mechanism actually does.) ;-) > Current version finds header.gcc in directories from GCC include directory search path but > fails to find them in subdirectories due to missing directory separator. > --- a/libcpp/files.c > +++ b/libcpp/files.c > @@ -1672,7 +1672,7 @@ static char * > remap_filename (cpp_reader *pfile, _cpp_file *file) > { > const char *fname, *p; > - char *new_dir; > + char *new_dir, *p3; > cpp_dir *dir; > size_t index, len; > > @@ -1701,9 +1701,15 @@ remap_filename (cpp_reader *pfile, _cpp_file *file) > return NULL; > > len = dir->len + (p - fname + 1); > - new_dir = XNEWVEC (char, len + 1); > + new_dir = XNEWVEC (char, len + 2); > + p3 = new_dir + dir->len; > memcpy (new_dir, dir->name, dir->len); > - memcpy (new_dir + dir->len, fname, p - fname + 1); > + if (dir->len && !IS_DIR_SEPARATOR(dir->name[dir->len - 1])) > + { > + *p3++ = '/'; > + len++; > + } > + memcpy (p3, fname, p - fname + 1); > new_dir[len] = '\0'; > > dir = make_cpp_dir (pfile, new_dir, dir->sysp); (I have not yet reviewed the logic of your change itself.) Wouldn't it be simpler to just unconditionally add a directory separator here? Is it OK to assume that a directory separator always is "/"? (Think DOS, Windows etc. But maybe there's some "translation layer" beneath this -- I don't know.) Can you provide some test cases? (Ah, I now see you got some "Test script to reproduce problem" attached to <https://gcc.gnu.org/PR71681> -- this should be turned into a regression test for the GCC testsuite.) It is good practice to assign a GCC PR to yourself if you're working on it, and it also helps to post to the PR a comment with a link to the mailing list archive for patch submissions, etc. Grüße Thomas
On 09/08/2016 12:09 PM, Thomas Schwinge wrote: > Hi! > > A few review comments: > > On Wed, 7 Sep 2016 20:19:20 +0300, Andris Pavenis <andris.pavenis@iki.fi> wrote: >> This patch fixes handling header.gcc in subdirectories when command line option -remap has been >> used. > (I have not yet reviewed the logic of your change itself.) Wouldn't it > be simpler to just unconditionally add a directory separator here? > > Is it OK to assume that a directory separator always is "/"? (Think DOS, > Windows etc. But maybe there's some "translation layer" beneath this -- > I don't know.) No. DJGPP supports both '/' and '\'. '\' is OK except in some cases (special handling of paths beginning with /dev/). Blind conversion off all '/' to '\' do not work for DJGPP due to this reason (had related problems in directory gcc/ada). Windows targets (WINGW, Cygwin): at least in Ada gcc/ada/s-os_lib.adb converts all '/' to '\' for Windows targets and without submitted but not yet committed patch also for DJGPP (that broke bootstrapping gcc for DJGPP due to gnatmake not working). I have not done however real testing for Windows targets myself. > Can you provide some test cases? (Ah, I now see you got some "Test > script to reproduce problem" attached to <https://gcc.gnu.org/PR71681> -- > this should be turned into a regression test for the GCC testsuite.) Which could more appropriate place for test-case? - gcc/testsuite/c-c++-common/cpp - gcc/testsuite/gcc.dg/cpp Also should this test be in a separate subdirectory under either of them? > It is good practice to assign a GCC PR to yourself if you're working on > it, and it also helps to post to the PR a comment with a link to the > mailing list archive for patch submissions, etc. Done Andris
From 77e02ba755fa9ea66e046ecf6dbc61c306bc2a71 Mon Sep 17 00:00:00 2001 From: Andris Pavenis <andris.pavenis@iki.fi> Date: Wed, 7 Sep 2016 18:22:32 +0300 Subject: [PATCH] * files.c (remap_filename): Fix handling -remap in subdirectories --- libcpp/files.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/libcpp/files.c b/libcpp/files.c index c8bb637..26a7330 100644 --- a/libcpp/files.c +++ b/libcpp/files.c @@ -1672,7 +1672,7 @@ static char * remap_filename (cpp_reader *pfile, _cpp_file *file) { const char *fname, *p; - char *new_dir; + char *new_dir, *p3; cpp_dir *dir; size_t index, len; @@ -1701,9 +1701,15 @@ remap_filename (cpp_reader *pfile, _cpp_file *file) return NULL; len = dir->len + (p - fname + 1); - new_dir = XNEWVEC (char, len + 1); + new_dir = XNEWVEC (char, len + 2); + p3 = new_dir + dir->len; memcpy (new_dir, dir->name, dir->len); - memcpy (new_dir + dir->len, fname, p - fname + 1); + if (dir->len && !IS_DIR_SEPARATOR(dir->name[dir->len - 1])) + { + *p3++ = '/'; + len++; + } + memcpy (p3, fname, p - fname + 1); new_dir[len] = '\0'; dir = make_cpp_dir (pfile, new_dir, dir->sysp); -- 2.7.4