-
Notifications
You must be signed in to change notification settings - Fork 217
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
implement ArcSort and refactor IsConnect with ConnectCore #19
Conversation
mm.. guys, can you kindly help to review this? |
I will review this to-night. |
Thanks!!!
…On Wed, Apr 29, 2020 at 5:22 PM Fangjun Kuang ***@***.***> wrote:
I will review this to-night.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/pull/19#issuecomment-621087730>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO3X5VVUGNC2MWFTEN3RO7WUBANCNFSM4MTOCQJA>
.
|
+2 |
Thanks! |
@@ -1,6 +1,7 @@ | |||
add_library(properties properties.cc) | |||
target_include_directories(properties PUBLIC ${CMAKE_SOURCE_DIR}) | |||
target_compile_features(properties PUBLIC cxx_std_11) | |||
target_link_libraries(properties PUBLIC fsa_algo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to add
target_link_libraries(fsa_algo PUBLIC properties)
since I want to use IsConnected()
in TopSort
;
but this results in circular dependencies:
CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
"properties" of type SHARED_LIBRARY
depends on "fsa_algo" (weak)
"fsa_algo" of type SHARED_LIBRARY
depends on "properties" (weak)
At least one of these targets is not a STATIC_LIBRARY. Cyclic dependencies are allowed only among static libraries.
Should we remove all add_library
and write a single add_library
like the following
add_library(fsa
fsa_equivalent.cc
fsa_util.cc
properties.cc
....
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this (maybe except fsa_render
?). Finally I guess we will have two .so
files for common FSA operations, one for CPU and another for GPU.
No description provided.