Remove an apparently bogus portability workaround (which was presumably
targetting one of the bugs fixed in the previous commit [1]) and further
simplify the code using std::find().
[1] da099d4312/
Explicitly enable the CMP0067 policy to have try_compile use the correct
C/C++ standard flags; otherwise, CMake will default to not honoring those,
causing the C/C++ checks to be compiled with no standard flags and SimGear
to be compiled with them. This causes errors if we try to detect a new
prototype which is only present in C++14 and above).
Indent the test code in a nicer way and shorten it, taking advantage of
this guarantee from the C++ standard:
If control reaches the end of main without encountering a return
statement, the effect is that of executing return 0.
-> no need for "#include <cstdlib>" nor for "return EXIT_SUCCESS".
Define HAVE_WORKING_STD_REGEX if, and only if <regex> is usable.
Normally, <regex> should be available and working in any compliant C++11
implementation, however at least g++ 4.8[1] lies about its C++11
compliance: its <regex> is utterly unusable, see [2] and [3] for
details.
[1] Which appears to be (precisely 4.8.5) the version shipped in
CentOS 7, and used on FlightGear's current Jenkins installation.
[2] https://stackoverflow.com/a/12665408/4756009
[3] https://sourceforge.net/p/flightgear/mailman/message/36170781/
Calling a virtual method in a destructor has undefined behaviour.
Furthermore passing 'this' to nasal from within the destructor
is not safe. 'onRemove' is called for widgets within layouts
on destruction anyhow, so no need to call it here again. If widgets
are arranged manually without any layout 'onRemoved' should be
called upon removing them from a dialog.
This allows one to use SGPath in containers such as std::map,
std::unordered_map and std::unordered_set.
Like the existing == and !=, all these operators rely solely on the
UTF-8 internal representation of the path.
We'll use this to cleanly declare std::hash<SGPath> as 'noexcept' in the
next commit, which appears to be desirable for std::hash template
specializations, according to:
http://en.cppreference.com/w/cpp/utility/hash
This is done by simply not user-defining the copy constructor,
copy-assignment operator and destructor. See [1] for more info.
[1] http://accu.org/content/conf2014/Howard_Hinnant_Accu_2014.pdf
For the benchmark below (compiled with the next commit to allow sorting
SGPath instances), on Linux amd64 with g++ 6.3.0, I observe that
enabling SGPath move operations with this commit increases the
performance by 31% or 28% respectively, depending on whether I use this:
// Typical code that creates a data structure in several steps and
// benefits from move operations (the std::move() does nothing when
// running the test with move operations disabled: a copy is made).
auto p = SGPath::fromUtf8(randomString(0, 30));
v.emplace_back(std::move(p));
or that:
v.emplace_back(randomString(0, 30))
for the initialization code. Now the benchmark code:
using std::string;
static std::default_random_engine randomNumbersGenerator;
// Utility function: generate a random string whose length is in the
// [minLen, maxLen] range.
string randomString(string::size_type minLen, string::size_type maxLen)
{
std::uniform_int_distribution<string::size_type> sLenDist(minLen, maxLen);
std::uniform_int_distribution<int> byteDist(0, 255);
auto randomByte = std::bind(byteDist, randomNumbersGenerator);
string::size_type len = sLenDist(randomNumbersGenerator);
string str;
while (str.size() < len) {
str += std::char_traits<char>::to_char_type(randomByte());
}
return str;
}
// The test function, run with nbIterations = 500000, minSize = 0 and
// maxSize = 200 to obtain the figures given above.
void SGPath_perfTest(std::size_t nbIterations,
std::size_t minSize, std::size_t maxSize)
{
std::uniform_int_distribution<std::size_t> sizeDist(minSize, maxSize);
auto randomSize = std::bind(sizeDist, randomNumbersGenerator);
std::chrono::time_point<std::chrono::system_clock> start, end;
start = std::chrono::system_clock::now();
vector<SGPath> v;
for (std::size_t i=0; i < nbIterations; i++) {
v = vector<SGPath>{}; // start anew
for (std::size_t j=0; j < randomSize(); j++) {
v.emplace_back(randomString(0, 30));
}
std::shuffle(v.begin(), v.end(), randomNumbersGenerator);
std::sort(v.begin(), v.end());
}
end = std::chrono::system_clock::now();
std::chrono::duration<double> elapsedSecs = end - start;
std::cout << elapsedSecs.count() << "\n"; // duration in seconds
}
With this simple change, the speedup as compared to commit 18f048424 is
now 37 % for the benchmark given in the previous commit. This is because
optimized swap() only needs to swap the raw pointers, which is certainly
less work than the three move assignments on SGSharedPtr (not raw
pointers) done by std::swap().
To benefit from this, write code like:
using std::swap; // now useless for SGSharedPtr, but idiomatic
swap(ptr1, ptr2); // *not* std::swap()!
This automatically makes SGSharedPtr more efficient when used in
standard containers (among others). See below for the benchmark details.
Mark as 'noexcept' (after checking it's legitimate!) the SGSharedPtr and
SGReferenced methods required for SGSharedPtr's move constructor and
move assignment operator to be guaranteed 'noexcept'.
Benchmark
---------
I measured a 25 % speedup with g++ 6.3.0 on Linux amd64, CFLAGS=-Wall -O2
as compared to commit 18f0484249 (which is
just before my changes to SGSharedPtr.hxx) on the following test code,
called with:
nbIterations = 3000000
minSize = 0
maxSize = 200
------------------------------------------------------------------------
static std::default_random_engine randomNumbersGenerator;
class SGReferencedTestClass : public SGReferenced
{ int i; };
void SGSharedPtr_perfTest(std::size_t nbIterations,
std::size_t minSize, std::size_t maxSize)
{
using Ref = SGSharedPtr<SGReferencedTestClass>;
std::uniform_int_distribution<std::size_t> sizeDist(minSize, maxSize);
auto randomSize = std::bind(sizeDist, randomNumbersGenerator);
std::chrono::time_point<std::chrono::system_clock> start, end;
start = std::chrono::system_clock::now();
std::vector<Ref> v;
for (std::size_t i=0; i < nbIterations; i++) {
v = std::vector<Ref>{}; // start anew
for (std::size_t j=0; j < randomSize(); j++) {
auto p = Ref(new SGReferencedTestClass());
v.emplace_back(std::move(p));
}
std::shuffle(v.begin(), v.end(), randomNumbersGenerator);
std::sort(v.begin(), v.end());
}
end = std::chrono::system_clock::now();
std::chrono::duration<double> elapsedSecs = end - start;
std::cout << elapsedSecs.count() << "\n"; // duration in seconds
}
------------------------------------------------------------------------
Basically, these gains can be explained by the fact that copying an
SGSharedPtr requires to test SGReferenced::ref, increase the refcount,
and then when the object is destroyed, test again SGReferenced::ref,
decrease the refcount and test it in order to maybe delete. With the
move constructor and move assignment operator, copying the argument is
never necessary: its raw pointer can be swapped with the one contained
in *this, which is very fast. For the move constructor, this is all that
is needed; move assignment just needs one reset() call after that in
order to release the resource from the moved-from shared pointer.
The copy-and-swap idiom is certainly very cute, but often causes
unnecessary copies. My commit fedafb9352
did exactly that, unfortunately.
Restore the exact same code for the copy-assignment operator as before
commit fedafb935, and add a more efficient implementation for the
move-assignment operator.
As explained by Howard Hinnant in [1] and [2], if some particular piece
of code really needs a strong exception safety guarantee, one can easily
add a specific method for that; this is not a valid reason to make the
code slower for all other places that have no use for such a guarantee!
[1] http://www.slideshare.net/ripplelabs/howard-hinnant-accu2014
[2] https://stackoverflow.com/a/9322542/4756009
