Re: Framework support for darwin

[Neil Booth <neil at daikokuya dot co dot uk>]

Mike Stump wrote:-

> I was worried that the data structures (_cpp_file) are private to 
> cpplib (just to cppfiles.c), and I didn't want to export them.  I think 
> we can export the notion as an opaque type and provide an accessor to 
> the data we want.  That way, I can improve the usability of the 
> callback to not do the loop and move the entire loop into the port 
> files.  These updates are reflected in the patch below.

Agreed, I didn't want to export _cpp_file either.  I think your accessor
idea is OK.  The whole thing is looking promising.

> >Whatever we decide I think most of the changes that are in c-incpath.c
> >should be in darwin.c.
> 
> Ok, gosh, no one wants it for Linux, sniff, I'll move em...

Nor NetBSD thanks...

> Ok, here is a updated patch...  Suggestions on improvements welcome.  
> Do don't really like the shape of the F stuff, the only problem, I 
> don't know what I like that is better.

What do you mean the shape of the F stuff?

I think we agree on where we want to go.  So here's a number of nits.
I'm protective of code I've just cleaned up a lot 8-)

> --- 305,318 ----
>   {
>     struct cpp_dir *p;
>   
> !   p = xmalloc (sizeof (cpp_dir));

Since it's referred to as struct cpp_dir immediately above, can we
not change this line?  I suspect it came from many revisions ago
in your own code...

> + #ifdef TARGET_EXTRA_INCLUDES
> +   TARGET_EXTRA_INCLUDES(stdinc);
> + #endif

This should be a target hook.  Yeah I know it's a bit more work,
but we're trying to kill these nasty macros.  This probably applies
to your -F handler too.

> *** ./c-incpath.h.~1~	Thu Jul 10 15:09:57 2003
> --- ./c-incpath.h	Thu Aug  7 17:37:15 2003
> *************** You should have received a copy of the G
> *** 15,23 ****
> --- 15,27 ----
>   along with this program; if not, write to the Free Software
>   Foundation, 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
>   
> + #include "cpplib.h"

Please don't include this - it appears the header only needs

struct _cpp_file;

and then the prototypes to use the struct-qualified form.  Faster
compiles!

> + F
> + C ObjC C++ ObjC++ Joined Separate
> + -F <dir>	Add <dir> to the end of the main framework include path  

I would have thought an -i switch would be canonical with all the other
CPP switches, and these single-letter switches are valuable 8-),
but I don't feel strongly enough to push it.

> *** 41,50 ****
> --- 41,60 ----
>   #include "ggc.h"
>   #include "langhooks.h"
>   #include "tm_p.h"
> + #include "c-incpath.h"
> + #include "cpplib.h"
> + #include "cpphash.h"
> + #include "c-pragma.h"

Including cpphash.h is "dame" as the Japanese would say.  Why did you
need to?

> *** 56,62 ****
>   /* This structure represents a file searched for by CPP, whether it
>      exists or not.  An instance may be pointed to by more than one
>      file_hash_entry; at present no reference count is kept.  */
> - typedef struct _cpp_file _cpp_file;

Can we leave this here?

>     else
> !     if (file->dir->construct)
> !       path = file->dir->construct (file->name, file->dir);
> !     else
> !       path = append_file_to_dir (file->name, file->dir);
>   
> !   if (path)
> !     {
> !       file->path = path;
> !       if (pch_open_file (pfile, file))
> ! 	return true;
>   
> !       if (open_file (file))
> ! 	return true;
> ! 
> !       free (path);
> !     }
> !   else
> !     {
> !       file->err_no = ENOENT;
> !     }

The ENOENT assignment was not necessary for append_file_to_dir,
and as you're not passing "file", I don't see why it should be
necessary for your callback either.  Can you explain?

If it is necessary, coding standards say we don't use {}.

> --- 370,391 ----
>   
>         if (file->err_no != ENOENT || (file->dir = file->dir->next) == NULL)
>   	{
> + 	  context_dependent_cb func = pfile->cb.context_dependent_header;
> + 
> + 	  /* When the regular search path doesn't work, try context dependent
> + 	     headers search paths.  */
> + 	  if (func
> + 	      && file->dir == NULL)
> + 	    {
> + 	      if ((file->path = func (pfile, fname)) != NULL)
> + 		{
> + 		  if (open_file (file))
> + 		    return file;
> + 		  free ((void *)file->path);
> + 		  file->path = NULL;
> + 		}
> + 	    }
> + 
>   	  open_file_failed (pfile, file);
>   	  break;
>   	}

Could you put this block in a separate function, say

{
  search_path_exhausted (pfile, file, fname);
  break;
}

I don't think a return value is needed, also note I don't "return;".
Doing that is not optimal as the found file is not then entered in
the hash table.

> + typedef struct _cpp_file _cpp_file;

Can this stay in cpphash.h?  I don't want to clutter the
unqualified namespace of the client who can't see the contents
anyway.

>     void (*def_pragma) (cpp_reader *, unsigned int);
>     int (*valid_pch) (cpp_reader *, const char *, int);
>     void (*read_pch) (cpp_reader *, const char *, int, const char *);
> +   context_dependent_cb context_dependent_header;
>   };

Can you think of a better name for this?  Maybe "missing_header"?

> *** 413,418 ****
> --- 423,434 ----
>        directories in the search path.  */
>     ino_t ino;
>     dev_t dev;
> + 
> +   /* Routine to construct pathname, given the search path name and the
> +      HEADER we are trying to find, return a constructed pathname to
> +      try and open.  If this is NULL, the constructed pathname is as
> +      constructed by append_file_to_dir.  */
> +   char *(*construct) (const char *header, cpp_dir *dir);

Please put this above ino and dev - they're tacked on the end as a
front-end kludge; the rest is really cpp-internal.  Also, can it
return "const char *"?

If you address the above I think this can go in if someone else is happy
with the Darwin stuff - I don't feel I can review it.

Neil.