Summary: | [CMAKE] Items of IDL list should be absolute path. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||
Component: | New Bugs | Assignee: | Ryuan Choi <ryuan.choi> | ||||
Status: | RESOLVED WONTFIX | ||||||
Severity: | Normal | CC: | paroga, rakuco, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 90258 | ||||||
Attachments: |
|
Description
Ryuan Choi
2012-07-01 18:25:09 PDT
Created attachment 150352 [details]
Patch
Comment on attachment 150352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150352&action=review Sorry, I didn't really understand your rationale for this change in https://bugs.webkit.org/show_bug.cgi?id=90258#c3 or in this bug. It makes more sense to just adjust the ADD_CUSTOM_COMMAND calls somehow than do this hack (there shouldn't be a need for some files to be specified as absolute paths and other files not in the CMake files). > Source/WebCore/PlatformBlackBerry.cmake:331 > -FOREACH (_idl ${WebCore_CPP_IDL_FILES}) > - SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > -ENDFOREACH () > +STRING(REPLACE ";" "\n" IDL_FILES_LIST "${WebCore_CPP_IDL_FILES}") This looks wrong; you're treating the list as a string here. Is there any particular reason to change this code? (In reply to comment #2) > (From update of attachment 150352 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150352&action=review > > Sorry, I didn't really understand your rationale for this change in https://bugs.webkit.org/show_bug.cgi?id=90258#c3 or in this bug. It makes more sense to just adjust the ADD_CUSTOM_COMMAND calls somehow than do this hack (there shouldn't be a need for some files to be specified as absolute paths and other files not in the CMake files). OK. I will close this. changing working directory is better. thank you. > > Source/WebCore/PlatformBlackBerry.cmake:331 > > -FOREACH (_idl ${WebCore_CPP_IDL_FILES}) > > - SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > > -ENDFOREACH () > > +STRING(REPLACE ";" "\n" IDL_FILES_LIST "${WebCore_CPP_IDL_FILES}") > > This looks wrong; you're treating the list as a string here. Is there any particular reason to change this code? As my curiosity, is list string which contains semicolons? I thought that this is the simplest way to make IDL_FILES_LIST if we don't need to insert ${WEBCORE_DIR} (In reply to comment #3) > > > Source/WebCore/PlatformBlackBerry.cmake:331 > > > -FOREACH (_idl ${WebCore_CPP_IDL_FILES}) > > > - SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > > > -ENDFOREACH () > > > +STRING(REPLACE ";" "\n" IDL_FILES_LIST "${WebCore_CPP_IDL_FILES}") > > As my curiosity, is list string which contains semicolons? > I thought that this is the simplest way to make IDL_FILES_LIST if we don't need to insert ${WEBCORE_DIR} According to CMake's documentation for the LIST() command, "A list in cmake is a ; separated group of strings" (so that code isn't really wrong). This might save a few lines compared to the original loop, but makes the meaning of the code less clear. (In reply to comment #4) > (In reply to comment #3) > > > > Source/WebCore/PlatformBlackBerry.cmake:331 > > > > -FOREACH (_idl ${WebCore_CPP_IDL_FILES}) > > > > - SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > > > > -ENDFOREACH () > > > > +STRING(REPLACE ";" "\n" IDL_FILES_LIST "${WebCore_CPP_IDL_FILES}") > > > > As my curiosity, is list string which contains semicolons? > > I thought that this is the simplest way to make IDL_FILES_LIST if we don't need to insert ${WEBCORE_DIR} > > According to CMake's documentation for the LIST() command, "A list in cmake is a ; separated group of strings" (so that code isn't really wrong). This might save a few lines compared to the original loop, but makes the meaning of the code less clear. Thank you for your kind explanation. I agree with you. |