compor / llvm-ir-cmake-utils

LLVM IR CMake utils for bitcode file manipulation by opt and friends
GNU Lesser General Public License v2.1
68 stars 14 forks source link

get absolute IN_FILE based on SOURCE_DIR of DEPENDS_TRGT #29

Closed tandf closed 1 year ago

tandf commented 1 year ago

Hi compor,

In llvmir_attach_bc_target, to get the absolute path of the in-file, currently get_filename_component is used: https://github.com/compor/llvm-ir-cmake-utils/blob/01fadd72be92c8342af299618ad71653746f4b72/cmake/LLVMIRUtil.cmake#L179

According to the doc, get_filename_component resolves the absolute path based on the CMAKE_CURRENT_SOURCE_DIR variable.

If no base directory is provided, the default base directory will be CMAKE_CURRENT_SOURCE_DIR.

However, the CMAKE_CURRENT_SOURCE_DIR variable doesn't always stand for the path where the DEPENDS_TRGT is defined. To handle the cases where CMAKE_CURRENT_SOURCE_DIR is messed up, I propose to instead explicitly look for the source path of DEPENDS_TRGT, i.e. use the SOURCE_DIR:

This read-only property reports the value of the CMAKE_CURRENT_SOURCE_DIR variable in the directory in which the target was defined.

For normal cases where the CMAKE_CURRENT_SOURCE_DIR variable is not messed up, it should contain the same value as SOURCE_DIR.

I have submitted a pull request for this change, see #28. Please let me know if you have any question. Thanks!

compor commented 1 year ago

@tandf thanks for the details explanation and sorry for the delay; I've left some minor comments.

tandf commented 1 year ago

@compor Thanks for the comments. I just added two replies to your comments.

compor commented 1 year ago

Hi again, sorry for being pedantic, but I'm in the unique position of not remembering the implementation very well ;-) so things like that stand out, so why not fix them eh?

tandf commented 1 year ago

Thanks for the comments. I just submitted a new commit for resolving the comments. For the details, please refer to the commit and my comments. Please help me review it when you have time. Thanks!

If it looks good to you, I can combine the two commits into one before merging this pr.

tandf commented 1 year ago

The commits are combined

compor commented 1 year ago

Addressed by #28 Thanks!