Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JITlink][AArch32] Move function declaration from unittest to headers #71621

Closed
wants to merge 1 commit into from

Conversation

tstellar
Copy link
Collaborator

@tstellar tstellar commented Nov 8, 2023

No description provided.

@tstellar tstellar changed the title [JITlink][AArch32] Add some misssing function declarations [JITlink][AArch32] Move function declaration from unittest to headers Nov 8, 2023
@weliveindetail
Copy link
Contributor

Hi Tom, these encoding functions are implementation details and weren't supposed to clutter the API. The idea was to only expose them for unit testing. Is there a policy or a technical reason that requires this change?

Copy link

github-actions bot commented Nov 8, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff d79fff0abb5a8ae6f49f5e3fd4f31d65717c7a37 c5dc8d00e33fce0807df678f9d603478d82b96e0 -- llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch32.h llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h llvm/unittests/ExecutionEngine/JITLink/AArch32Tests.cpp
View the diff from clang-format here.
diff --git a/llvm/unittests/ExecutionEngine/JITLink/AArch32Tests.cpp b/llvm/unittests/ExecutionEngine/JITLink/AArch32Tests.cpp
index 7735a68d01b4..3f08a4b95f07 100644
--- a/llvm/unittests/ExecutionEngine/JITLink/AArch32Tests.cpp
+++ b/llvm/unittests/ExecutionEngine/JITLink/AArch32Tests.cpp
@@ -7,8 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include <llvm/BinaryFormat/ELF.h>
-#include <llvm/ExecutionEngine/JITLink/aarch32.h>
 #include <llvm/ExecutionEngine/JITLink/ELF_aarch32.h>
+#include <llvm/ExecutionEngine/JITLink/aarch32.h>
 
 #include "gtest/gtest.h"
 

@tstellar
Copy link
Collaborator Author

tstellar commented Nov 8, 2023

@weliveindetail I'm trying to add visibility attributes to all used functions and classes in libLLVM.so. So far I've been been adding annotations to headers only, so I need to have the declarations visible when the implementation is complied so that the visibility attribute is applied.

If we don't want to expose these functions to the API, then I think I can special case them and just add the visibility attributes on the implementation.

@weliveindetail
Copy link
Contributor

If we don't want to expose these functions to the API, then I think I can special case them and just add the visibility attributes on the implementation.

Yes, would be great if that's possible. They should all have visibility("hidden") I think.

@tstellar tstellar closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants