Fix symbol visibility issues for C API #440

Gnimuc · 2025-01-12T11:00:51Z

Add the missing CINDEX_LINKAGE macros for C APIs

github-actions · 2025-01-12T11:04:20Z

clang-tidy review says "All clean, LGTM! 👍"

codecov · 2025-01-12T11:10:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.15%. Comparing base (54c9482) to head (f2d2dd9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #440   +/-   ##
=======================================
  Coverage   71.15%   71.15%           
=======================================
  Files           9        9           
  Lines        3550     3550           
=======================================
  Hits         2526     2526           
  Misses       1024     1024

mcbarton · 2025-01-12T11:26:45Z

@Gnimuc Your PR which added the cpi was largely untested (see #438 (comment)). Would you mind putting in a PR which tests your C API so can get our total test coverage back up?

Gnimuc · 2025-01-12T12:12:07Z

@Gnimuc Your PR which added the cpi was largely untested (see #438 (comment)). Would you mind putting in a PR which tests your C API so can get our total test coverage back up?

@mcbarton

Half of the untested lines are copied from libclang:

https://app.codecov.io/gh/compiler-research/CppInterOp/blob/main/lib%2FInterpreter%2FCXCppInterOp.cpp#L156

Since those C interfaces are just thin wrappers, I think a more practical way to improve the coverage of the project is adding CXCppInterOp.cpp to the ignore path of CodeCov.

mcbarton · 2025-01-12T12:20:44Z

@Gnimuc Your PR which added the cpi was largely untested (see #438 (comment)). Would you mind putting in a PR which tests your C API so can get our total test coverage back up?

@mcbarton

Half of the untested lines are copied from libclang:

https://app.codecov.io/gh/compiler-research/CppInterOp/blob/main/lib%2FInterpreter%2FCXCppInterOp.cpp#L156

Since those C interfaces are just thin wrappers, I think a more practical way to improve the coverage of the project is adding CXCppInterOp.cpp to the ignore path of CodeCov.

@vgvassilev What do you think of @Gnimuc suggestion?

vgvassilev · 2025-01-12T14:09:11Z

I don't have a strong opinion. Generally it is good to have tests especially for the things that are not in libclang.

mcbarton · 2025-01-12T15:17:15Z

@Gnimuc I gave it some thought and think it would still be better to have the tests. Hopefully given they are just thin wrappers then I believe it shouldn't be too much to get tests written with them too. Then we can guarantee everything is working as expected.

github-actions · 2025-01-17T08:03:31Z

clang-tidy review says "All clean, LGTM! 👍"

Gnimuc · 2025-01-19T07:11:12Z

@vgvassilev Is it OK to turn off warning C4273 as error?

vgvassilev

Lgtm modulo the comment.

vgvassilev · 2025-01-19T07:15:33Z

@vgvassilev Is it OK to turn off warning C4273 as error?

Do we understand why the warning fires and why it is not relevant?

github-actions

clang-tidy made some suggestions

github-actions · 2025-01-19T09:46:45Z

lib/Interpreter/CXCppInterOp.cpp

@@ -270,7 +270,8 @@ static inline compat::Interpreter* getInterpreter(const CXInterpreterImpl* I) {
  return I->Interp.get();
 }

-CXInterpreter clang_createInterpreter(const char* const* argv, int argc) {
+CINDEX_LINKAGE CXInterpreter clang_createInterpreter(const char* const* argv,


warning: no header providing "CINDEX_LINKAGE" is directly included [misc-include-cleaner]

lib/Interpreter/CXCppInterOp.cpp:15:

- #include <cstring> + #include <clang-c/Platform.h> + #include <cstring>

github-actions · 2025-01-19T09:46:45Z

lib/Interpreter/CXCppInterOp.cpp

  return static_cast<CXInterpreterImpl*>(I)->Interp.release();
 }

-enum CXErrorCode clang_Interpreter_undo(CXInterpreter I, unsigned int N) {
+CINDEX_LINKAGE enum CXErrorCode clang_Interpreter_undo(CXInterpreter I,


warning: no header providing "CXErrorCode" is directly included [misc-include-cleaner]

lib/Interpreter/CXCppInterOp.cpp:15:

- #include <cstring> + #include <clang-c/CXErrorCode.h> + #include <cstring>

github-actions · 2025-01-19T09:46:45Z

lib/Interpreter/CXCppInterOp.cpp

@@ -584,22 +588,26 @@
  return clang::cxscope::MakeCXScope(D, getNewTU(tmpl));
 }

-CXObject clang_allocate(unsigned int n) { return ::operator new(n); }
+CINDEX_LINKAGE CXObject clang_allocate(unsigned int n) {
+  return ::operator new(n);


warning: no header providing "operator new" is directly included [misc-include-cleaner]

lib/Interpreter/CXCppInterOp.cpp:17:

- #include "clang-c/CXString.h" + #include <new> + #include "clang-c/CXString.h"

github-actions · 2025-01-19T09:46:45Z

lib/Interpreter/CXCppInterOp.cpp


-void clang_deallocate(CXObject address) { ::operator delete(address); }
+CINDEX_LINKAGE void clang_deallocate(CXObject address) {
+  ::operator delete(address);


warning: no header providing "operator delete" is directly included [misc-include-cleaner]

::operator delete(address); ^

github-actions · 2025-01-19T09:55:31Z

clang-tidy review says "All clean, LGTM! 👍"

Gnimuc · 2025-01-19T10:02:11Z

@vgvassilev Is it OK to turn off warning C4273 as error?

Do we understand why the warning fires and why it is not relevant?

It turns out that the CINDEX_LINKAGE is not correctly triggered in the CMake configuration. I added a small fix to work around the problem.

github-actions · 2025-01-19T10:02:14Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2025-01-19T10:08:05Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2025-01-19T16:46:50Z

lib/Interpreter/CMakeLists.txt

@@ -133,6 +133,8 @@ else()
    LINK_LIBS
    ${link_libs}
  )
+
+  target_compile_definitions(clangCppInterOp PUBLIC "_CINDEX_LIB_") # workaround for the use of `CINDEX_LINKAGE`


How about using CLANG_ABI from clang/Support/Compiler.h?

Gnimuc mentioned this pull request Jan 12, 2025

libCppInterOp: fix symbol visibility issues on Windows JuliaPackaging/Yggdrasil#10250

Merged

Gnimuc mentioned this pull request Jan 13, 2025

Improve the test coverage of the C API #447

Open

4 tasks

vgvassilev force-pushed the c-api-visibility branch from 1b511ce to 2b203c4 Compare January 17, 2025 08:00

Fix symbol visibility issues for C API

0db7431

Gnimuc force-pushed the c-api-visibility branch from 2b203c4 to 0db7431 Compare January 19, 2025 06:08

vgvassilev approved these changes Jan 19, 2025

View reviewed changes

github-actions bot reviewed Jan 19, 2025

View reviewed changes

Gnimuc force-pushed the c-api-visibility branch from 3d44235 to d7e0fc7 Compare January 19, 2025 09:49

Add a workaround for triggering the use of CINDEX_LINKAGE

f2d2dd9

Gnimuc force-pushed the c-api-visibility branch from d09dd1b to f2d2dd9 Compare January 19, 2025 10:03

vgvassilev reviewed Jan 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symbol visibility issues for C API #440

Fix symbol visibility issues for C API #440

Gnimuc commented Jan 12, 2025

github-actions bot commented Jan 12, 2025

codecov bot commented Jan 12, 2025 •

edited

Loading

mcbarton commented Jan 12, 2025 •

edited

Loading

Gnimuc commented Jan 12, 2025

mcbarton commented Jan 12, 2025

vgvassilev commented Jan 12, 2025

mcbarton commented Jan 12, 2025

github-actions bot commented Jan 17, 2025

Gnimuc commented Jan 19, 2025

vgvassilev left a comment •

edited

Loading

vgvassilev commented Jan 19, 2025

github-actions bot left a comment

github-actions bot Jan 19, 2025

github-actions bot Jan 19, 2025

github-actions bot Jan 19, 2025

github-actions bot Jan 19, 2025

github-actions bot commented Jan 19, 2025

Gnimuc commented Jan 19, 2025 •

edited

Loading

github-actions bot commented Jan 19, 2025

github-actions bot commented Jan 19, 2025

vgvassilev Jan 19, 2025

Fix symbol visibility issues for C API #440

Are you sure you want to change the base?

Fix symbol visibility issues for C API #440

Conversation

Gnimuc commented Jan 12, 2025

github-actions bot commented Jan 12, 2025

codecov bot commented Jan 12, 2025 • edited Loading

Codecov Report

mcbarton commented Jan 12, 2025 • edited Loading

Gnimuc commented Jan 12, 2025

mcbarton commented Jan 12, 2025

vgvassilev commented Jan 12, 2025

mcbarton commented Jan 12, 2025

github-actions bot commented Jan 17, 2025

Gnimuc commented Jan 19, 2025

vgvassilev left a comment • edited Loading

Choose a reason for hiding this comment

vgvassilev commented Jan 19, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 19, 2025

Choose a reason for hiding this comment

github-actions bot Jan 19, 2025

Choose a reason for hiding this comment

github-actions bot Jan 19, 2025

Choose a reason for hiding this comment

github-actions bot Jan 19, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2025

Gnimuc commented Jan 19, 2025 • edited Loading

github-actions bot commented Jan 19, 2025

github-actions bot commented Jan 19, 2025

vgvassilev Jan 19, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2025 •

edited

Loading

mcbarton commented Jan 12, 2025 •

edited

Loading

vgvassilev left a comment •

edited

Loading

Gnimuc commented Jan 19, 2025 •

edited

Loading