Private headers and testing

Hi all,

Reading this doc: http://llvm.org/docs/CodingStandards.html#hl_privateheaders, it suggests putting private implementation details outside of llvm/include/* to avoid polluting the public header space. This makes sense, and it works fine, because make enters every directory and hence doesn’t need a -I path to include them.

However, unittests need to also #include these headers to be able to test the internal implementations, so here are potential ways to do this:

  1. Leave headers where they are now; pass in -I$(LLVM_SRC_ROOT) and #include “lib/Transform/…” for the unittests
  2. Same as above, except with -I$(LLVM_SRC_ROOT)/lib and #include “Transform/…”
  3. Create a new header directory for internal headers, e.g. llvm/include/llvm/internal/ and put private headers there; no one needs any new -I switches, implementation libraries and unittests all say #include “llvm/internal/*” and all is well. If there’s a concern that we are distributing/installing private headers, we can amend the install script to “rm -rf llvm/include/internal” to avoid the issue.

Thoughts/comments?

Misha
(I’m voting #3)

Do you have a specific example of a unit test that would need these? I really think these should stay private.

-Chris

2009/1/2 Chris Lattner <clattner@apple.com>

Hi all,

Reading this doc: http://llvm.org/docs/CodingStandards.html#hl_privateheaders, it suggests putting private implementation details outside of llvm/include/* to avoid polluting the public header space. This makes sense, and it works fine, because make enters every directory and hence doesn’t need a -I path to include them.

Conceptually, this kind of test should have the same include path view
of the source tree as the source it’s testing. Would it be possible to create
a parallel directory structure under the unittests directory? That way, a test
in unittests/lib/Transform/… could
be given -I$(LLVM_SRC_ROOT)/lib/Transform/… .

However, unittests need to also #include these headers to be able to test the internal implementations, so here are potential ways to do this:

  1. Leave headers where they are now; pass in -I$(LLVM_SRC_ROOT) and #include “lib/Transform/…” for the unittests
  2. Same as above, except with -I$(LLVM_SRC_ROOT)/lib and #include “Transform/…”
  3. Create a new header directory for internal headers, e.g. llvm/include/llvm/internal/ and put private headers there; no one needs any new -I switches, implementation libraries and unittests all say #include “llvm/internal/*” and all is well. If there’s a concern that we are distributing/installing private headers, we can amend the install script to “rm -rf llvm/include/internal” to avoid the issue.

Otherwise, if we’re voting, I vote for #2, because it keeps the tree a little
tidier. Also, LLVM is used in diverse settings with a variety of install scripts,
so it’s preferable to keep things simple.

Dan

Let's take lib/Transforms/Utils/CodeExtractor.cpp . The public interface for it is in include/llvm/Transform/Utils/FunctionUtils.h, with only the high-level API.

I believe this should be possible to handle within GNU Make. Assuming that the source file in question is ‘TheTest.cpp’, something like this should do the trick:
$(ObjDir)/TheTest.o $(ObjDir)/TheTest.lo: \
  CompileCommonOpts += -I$(PROJ_SRC_ROOT)/lib/Transforms/Utils
The specific entry in the GNU Make manual is ‘6.10 Target-specific Variable Values’. [1] (I used this when trying to decrease the amount of libraries in LLVM; an example can be seen at [2].)

I hope this helps :slight_smile:

[1] <http://www.gnu.org/software/make/manual/make.html#Target_002dspecific >
[2] <http://www.bitbucket.org/danchr/llvm/src/ab1e5398289e/lib/Target/Makefile#cl-59

2009/1/2 Chris Lattner <clattner@apple.com>

Do you have a specific example of a unit test that would need these? I really think these should stay private.

Let’s take lib/Transforms/Utils/CodeExtractor.cpp . The public interface for it is in include/llvm/Transform/Utils/FunctionUtils.h, with only the high-level API.

If I want to test some corner-case (consider the number of code paths in that code), I have to strain to come up with some combination of a Function and input BasicBlocks that will fit all the conditions from the outset. Or, I can just feed each individual method in the class exactly the corner cases I will want it to handle, and verify its output (or side effects) exactly, to make sure I pin-pointed the issue.

Sure, ok. But that is a public API, I’m specifically interested in cases you need private API.

Or, let’s say I want to write some tests for the LLParser – the only public API is “ParseAssemblyString”, I can’t unittest each individual method. The unittest for this using the public API will be very brittle as it would have to check the contents of the returned error message, instead of calling each Parse*() function directly and analyzing its output.

The code is structured so that it is trivially easy to construct testcases for anything you’re concerned with. If you give me an example, I’ll give you a testcase (in .ll form). I don’t see the need for unittests for LLParser.

-Chris

2009/1/3 Chris Lattner <clattner@apple.com>

I am pretty strongly against that. Specifically for CodeExtractor, what benefit do you gain by doing this?

-Chris