[cmake-developers] INCLUDE_DIRECTORIES variable expansion

[Stephen Kelly]

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.