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) -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090102/1f334826/attachment.html>
On Jan 2, 2009, at 12:21 PM, Misha Brukman wrote:> 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.Do you have a specific example of a unit test that would need these? I really think these should stay private. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090102/ead1622b/attachment.html>
2009/1/2 Chris Lattner <clattner at apple.com>> On Jan 2, 2009, at 12:21 PM, Misha Brukman wrote: > 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. 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. Of course, we should still have a test for the overall public API, but that's in addition to the small unit tests to make sure each one works as it should. Note that this is a parallel to having an on-disk representation of IR (which LLVM has) to a lack of such (which some other compilers had and/or have now). In the former case, to test an optimization, one just writes LLVM IR and runs the pass directly on the case at hand. In the other case, one has to strain to come up with a front-end input source that will trigger something in the 'blackbox' middle or back-end code, knowing that it will get pre-digested before it gets there, and hence it's hard to know what you're really testing on. Misha -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090102/d46222fa/attachment.html>
On Jan 2, 2009, at 12:21 PM, Misha Brukman wrote:> 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090103/09db458b/attachment.html>