google / szl

A compiler and runtime for the Sawzall language
Other
69 stars 16 forks source link

Public interface headers include a non-public header #13

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
According to a comment in ``sawzall.h``, the following files are Sawzall's 
public interface:

   porting.h
   sawzall.h
   emitterinterface.h
   szltype.h
   szlvalue.h
   szlresults.h
   szlencoder.h
   szldecoder.h
   szlnamedtype.h
   value.h

Some of those files, however, do a ``#include "public/szltype.h"``, which does 
not work outside Sawzall's source tree.

The attached diff addresses this by removing the ``#include 
"public/szltype.h"`` line from the header files, and adding it to the source 
files that need it.

I don't like the fact that now the order of included files matters (you have to 
include ``szltype.h`` before including ``szlresults.h``, for instance). Any 
idea on a better approach?

Original issue reported on code.google.com by superdup...@gmail.com on 11 Nov 2010 at 7:15

Attachments:

GoogleCodeExporter commented 9 years ago
I think you're expected to compile with -I/usr/local/include/google, so that 
the includes work as they are.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Order_o
f_Includes says that each header file should be compilable on its own, so I 
don't think a patch that removes essential includes is likely to be accepted.  
Not that it's my decision, you understand.

 --Adrian.

Original comment by aecolley on 13 Nov 2010 at 2:29

GoogleCodeExporter commented 9 years ago
> I think you're expected to compile with -I/usr/local/include/google, so that
> the includes work as they are.

I'm afraid that would not work, Adrian: The problematic includes are:

  #include "public/szltype.h"

and there's no ``public`` directory under ``${prefix}/include/google``. If 
``szltype.h`` is to be included safely in other public Sawzall header files, I 
suppose it should be done something like:

  #include <google/szl/szltype.h>

Or just as ``#include <szltype.h>``, with ``-I${prefix}/include/google/szl`` 
added to the required CPPFLAGS.

> 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Order_o
f_Includes
> says that each header file should be compilable on its own, so I don't think
> a patch that removes essential includes is likely to be accepted.

Yep, you're right on that (thanks for the link, by the way!) The patch as it
stands now is probably worthless.

Carlos

Original comment by superdup...@gmail.com on 13 Nov 2010 at 2:56

GoogleCodeExporter commented 9 years ago
Another approach: Add the following to the CPP flags:

  -DIN_SZL_SRC_TREE

and use the appropriate header file path in public headers:

  #ifdef IN_SZL_SRC_TREE
  #include "public/szltype.h"
  #else
  #include <google/szl/szltype.h>
  #endif

Original comment by superdup...@gmail.com on 26 Dec 2010 at 1:09

Attachments:

GoogleCodeExporter commented 9 years ago
By analogy with protobuf, I've renamed public/ to google/szl and installed it 
with "nobase_include_HEADERS". Patch (against svn r53) attached.

Original comment by aecolley on 17 Oct 2013 at 2:17

Attachments: