[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/test/CodeGen/debug-info-extern-basic.c:14-23
+extern int (*foo)(int);
+int test3() {
+  return foo(0);
+}
+
+int test4() {
+  extern int (*foo2)(int);
----------------
yonghong-song wrote:
> dblaikie wrote:
> > What do these tests add? What sort of bugs would be caught by these global function pointer tests that wouldn't be caught by the char tests above them?
> These two additional tests to test extern function pointers. One of bpf program use cases specifically need extern function debuginfo type so I added a bunch. I do agree that this may be too much unnecessary and variable tests should cover this.
> 
> I will remove all these extern function pointer tests including below one, except one like the above test3() which should be enough.
Just to explain my thinking a bit more clearly here:

Testing this feature shouldn't be related to how you want to use the feature, but how the feature's designed/what likely places it could have bugs.

If the feature creates types in the same way for every type, and in a way that's already well tested for other types - then I don't think it's useful to test more than one type here.

(imagine, say, testing code generation for function calls - function calls are independent of the expressions that generate their arguments - so it doesn't make sense to test "foo(3)" and "foo(1 + 2)", etc - and of course there are limits to this sort of "orthogonality" analysis, taken too far you don't get enough test coverage, etc - but generally that's what I'm thinking of when I'm thinking about test case reduction, is semi-white-box "could I introduce a bug that would make one of these tests fail and teh other pass" - if it's pretty clear that the two systems are independent, and each independently well tested, I'll only test one or two paths through both of the systems)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70696/new/

https://reviews.llvm.org/D70696