Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
Brad King wrote: IMO the policy is worthwhile anyway. I reverted your previous topic using the above approach and replaced it with this: http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=2a0a5c78 I suggest renaming the new policy to the CMP0019 spot and not reserving any policies that won't be in the same release as that commit. Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
On 12/07/2012 06:12 AM, Stephen Kelly wrote: Brad King wrote: IMO the policy is worthwhile anyway. I reverted your previous topic using the above approach and replaced it with this: http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=2a0a5c78 I suggest renaming the new policy to the CMP0019 spot and not reserving any policies that won't be in the same release as that commit. Okay. I added this commit to 'next' to rename it: http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f3953103 though that will never go to 'master'. Instead I squashed it: http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=711b63f7 Please rebase your other topics and renumber the policies. Thanks, -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
On 11/29/2012 02:48 PM, Stephen Kelly wrote:
So maybe I should only do the expansion if containsVariable is true?
+//
+static bool containsVariable(const std::string lib)
+{
+ const std::string::size_type openpos = lib.find(${);
+ return (openpos != std::string::npos)
+ (lib.find(}, openpos) != std::string::npos);
+}
Then I wouldn't need a policy.
IMO the policy is worthwhile anyway. I reverted your previous
topic using the above approach and replaced it with this:
http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=2a0a5c78
-Brad
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
Brad King wrote:
On 11/29/2012 02:48 PM, Stephen Kelly wrote:
So maybe I should only do the expansion if containsVariable is true?
+//
+static bool containsVariable(const std::string lib)
+{
+ const std::string::size_type openpos = lib.find(${);
+ return (openpos != std::string::npos)
+ (lib.find(}, openpos) != std::string::npos);
+}
Then I wouldn't need a policy.
IMO the policy is worthwhile anyway. I reverted your previous
topic using the above approach and replaced it with this:
http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=2a0a5c78
Fair enough. :) Thanks.
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion (was: Performance problem in tll-set-include-and-defines-requirements)
On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion (was: Performance problem in tll-set-include-and-defines-requirements)
Brad King wrote: On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. Ok, I'll wait until we hear from Dave so I can write a well-informed commit message. Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion (was: Performance problem in tll-set-include-and-defines-requirements)
Stephen Kelly wrote: Brad King wrote: On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. Ok, I'll wait until we hear from Dave so I can write a well-informed commit message. I guess I'll also hit a problem with the policy number to use. In my branches in next I introduce CMP0019 and CMP0020. For simplicity, can I make this CMP0021 and make the topic depend on the tll-set-include-and-defines-requirements topic? Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion (was: Performance problem in tll-set-include-and-defines-requirements)
Looking at this now, will reply in just a minute On Thu, Nov 29, 2012 at 2:14 PM, Stephen Kelly steve...@gmail.com wrote: Brad King wrote: On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. Ok, I'll wait until we hear from Dave so I can write a well-informed commit message. Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion (was: Performance problem in tll-set-include-and-defines-requirements)
Brad King wrote: On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. Hmm, I guess that will mean that http://thread.gmane.org/gmane.comp.kde.devel.buildsystem/7584 will not be fixed, because KDE does not set the policy to NEW. To fix that, I would need to not do the expansion by default somehow. Is there any way to do that? Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
On 11/29/2012 02:17 PM, Stephen Kelly wrote: For simplicity, can I make this CMP0021 and make the topic depend on the tll-set-include-and-defines-requirements topic? Yes, thanks. -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion (was: Performance problem in tll-set-include-and-defines-requirements)
Before we do another policy here, does the performance problem go away if you comment out the SetProperty calls? If so, we can just add some code that detects whether any variables were actually expanded in the call, and then only call SetProperty when that's the case. (In the vast majority of cases there will not be any variables in the property values...) The code is left in there simply because it was there before the moving includes to a target property change. It is still unclear to me why that code is required. I suspect it is probably not at this point, but since I don't understand the full history there, I can't state that with 100% certainty. This is a problem only now that generator expressions are use-able in this context. If there were no $ in the input, then ExpandVariablesInString would return immediately without doing any significant work... On Thu, Nov 29, 2012 at 2:30 PM, David Cole david.c...@kitware.com wrote: Looking at this now, will reply in just a minute On Thu, Nov 29, 2012 at 2:14 PM, Stephen Kelly steve...@gmail.com wrote: Brad King wrote: On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. Ok, I'll wait until we hear from Dave so I can write a well-informed commit message. Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
On 11/29/2012 02:31 PM, Stephen Kelly wrote: Brad King wrote: On 11/29/2012 07:34 AM, Stephen Kelly wrote: The patch that added that line is d899eb71b52616c9e3f3f5987f229619605e632b. It is apparently there for strict backwards compatibility, but it is not clear what exactly needs to be backwards compatible there. Any idea about that? I don't know off the top of my head, Dave? Stephen, please add the policy that the d899eb71 commit message suggests. Only bother with expansion if the policy is OLD or not set. Hmm, I guess that will mean that http://thread.gmane.org/gmane.comp.kde.devel.buildsystem/7584 will not be fixed, because KDE does not set the policy to NEW. To fix that, I would need to not do the expansion by default somehow. Is there any way to do that? If the expansion has always been done before why is it suddenly so slow? Is it because memoization was dropped? -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
David Cole wrote:
Before we do another policy here, does the performance problem go away if
you comment out the SetProperty calls?
Nope. The performance problem is the expansion by ExpandVariablesInString.
See the patch I posted with the testcase:
http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/5405/focus=5406
If so, we can just add some code that detects whether any variables were
actually expanded in the call, and then only call SetProperty when that's
the case. (In the vast majority of cases there will not be any variables
in the property values...)
The code is left in there simply because it was there before the moving
includes to a target property change.
Perhaps. That could then be a 'backwards compatibility trap' in my opinion,
which compromises flexibility in the future, as I've talked about in other
threads. These kinds of backward compatibility patches need to be understood
fully before being applied, I think. I think they're more important than any
other patch because they affect future flexibility and bugs so much.
Sorry for bringing this up again, but I've been bitten by it several times
now.
It is still unclear to me why that code is required. I suspect it is
probably not at this point, but since I don't understand the full history
there, I can't state that with 100% certainty.
This is a problem only now that generator expressions are use-able in this
context. If there were no $ in the input, then ExpandVariablesInString
would return immediately without doing any significant work...
That would explain the performance degredation then.
So maybe I should only do the expansion if containsVariable is true?
+//
+static bool containsVariable(const std::string lib)
+{
+ const std::string::size_type openpos = lib.find(${);
+ return (openpos != std::string::npos)
+ (lib.find(}, openpos) != std::string::npos);
+}
?
Then I wouldn't need a policy.
Thanks,
Steve.
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] INCLUDE_DIRECTORIES variable expansion
Brad King wrote: If the expansion has always been done before why is it suddenly so slow? Is it because memoization was dropped? Nope, it appears to be the '$' characters in strings which contain things like: $$TARGET_DEFINED:foo:$TARGET_PROPERTY:foo,INCLUDE_DIRS The testcase I posted before shows the performance degredation when they are present. Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
