Developer forums (C::B DEVELOPMENT STRICTLY!) > CodeCompletion redesign
F() function is not thread safe?
thomas:
Well, that's a namespace which you're not supposed to use, ever. And sure I wouldn't want a variable called s in global namespace. :D
It's only there at all to provide those two static strings for the function that follows, so we're not constructing these strings a thousand times per second.
Though of course we do a few other things in the logger which aren't precisely high-perforance, such as outputting single newlines and changing style twice in between... that generates about a dozen window messages inside wxWidgets per line of output (yuck). But I guess we'll leave that for another day.
Calling the namespace pfffffffrt seems discouraging enough, doesn't it? I was tempted to call it _________ for a while, at the risk of being yelled at because double-underscore names are reserved ;D
oBFusCATed:
Some C++-devs use detail for such namespaces.
Although, why would you want to have s and sl as global static consts in the first place?
Another problem with your patch is that you're using tabs instead of spaces :P
And the final problem, the F should be changed to return wxString instead of const wxString&.
p.s. this function is perfect candidate for RVO, so probably the compiler omits the copying.
thomas:
As explained, I've put in these variables because someone is using wxString::Replace with a const char* argument for some weird wx 2.9 compatibility hack. I don't even get why that hack is necessary, but obviously the person who thought of it had a good reason for it.
Unluckily, wxString::Replace takes wxString arguments, not const char*, which means that for every string you format, you create two unnecessary temporaries just to do this replacement. Of course you can easily do without these, and construct/destruct even a few more objects. But seeing how it's happening many thousands of times, I deemed it worthwhile. We can of course do without if you don't like it.
So what name other than s would better describe a constant string with the contents "s" :)
As for tabs... yeah, sorry... I have the editor set to use tabs, keep forgetting that C::B sources have the silly "use spaces" notation. About RVO I'm not sure if it works. Unluckily we can't return an anonymous temporary because of var_end which must appear before return but after any var args are consumed (i.e. to construct the returned object). Not sure if the compiler is smart enough to elide the copy anyway.
oBFusCATed:
--- Quote from: thomas on January 09, 2014, 09:56:47 pm ---So what name other than s would better describe a constant string with the contents "s" :)
--- End quote ---
You can make them local functions statics, I think...
--- Quote from: thomas on January 09, 2014, 09:56:47 pm ---Not sure if the compiler is smart enough to elide the copy anyway.
--- End quote ---
Compiler writers, tells us that they are smart enough... http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/
Do you agree that the return type should be changed?
thomas:
--- Quote from: oBFusCATed on January 10, 2014, 12:50:15 am ---You can make them local functions statics, I think...
--- End quote ---
Yes, except function local statics effectively result in a sort of if(initialized) foo = initializer; code sequence, and in C++11 these are required to be initialized thread-safe, and GCC since about version 4.6 or 4.7 does that regardless of C++11 anyway (unless you compile with -fno-threadsafe-statics), using a mutex. Now of course, locking a mutex makes the point of trying to avoid initilizing a string somewhat... redundant.
--- Quote ---
--- Quote from: thomas on January 09, 2014, 09:56:47 pm ---Not sure if the compiler is smart enough to elide the copy anyway.
--- End quote ---
Compiler writers, tells us that they are smart enough... http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/
Do you agree that the return type should be changed?
--- End quote ---
I'm know this article, but I'm not convinced that it applies to us. For example, we don't follow the guideline of not copying arguments (obviously) and your return value is not anonymous, so at best NRVO can be used, not RVO. Also, var-args are a kind of ugly hack, and I'm not sure inhowfar it influences optimizations such as NRVO.
But I'm OK if you think that returning by value is a better choice, even if it does create a copy. A verbatim copy on a wxString is only incrementing a reference counter.
Feel free to choose whatever makes you feel most comfortable.
What worries me more than those micro-optimizations is code like this:
--- Code: ---void TextCtrlLogger::Append(const wxString& msg, Logger::level lv)
{
if (!control)
return;
::temp_string.assign(msg);
::temp_string.append(_T("\n"));
if (lv == caption)
{
control->SetDefaultStyle(style[info]);
control->AppendText(::newline_string);
control->SetDefaultStyle(style[lv]);
control->AppendText(::temp_string);
control->SetDefaultStyle(style[spacer]);
control->AppendText(::newline_string);
}
else
{
control->SetDefaultStyle(style[lv]);
control->AppendText(::temp_string);
}
}
--- End code ---
The F function as it was is kind of "semi threadsafe" insofar as all modules that were supposed to use it (mostly compiler, and maybe project mgr) run at most one thread per module. Now the TextCtrlLogger such as being used for the debug log on the other hand is definitely thread-unsafe, but I don't even understand why it uses the global here at all. Appending a newline would make much more sense on a plain normal local variable, no?
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version