Small stuff #305
Small stuff #305
Conversation
Doesn't change any actual behaviour, but stops tools like clang-tidy complaining about possibly missed values.
ftell() always returns a long, there's no bug here. But, `long` is a tricky type - sometimes 32bit, sometimes 64bit, thus it's generally advisable to avoid it and static analysis tools like to warn about it and recommend using fixed size integers - I think a better option is to just use `auto`. The end result is the same (`long` is deduced) but it shuts up static analyzers and it's no less readable (IMHO).
Just to make the style more consistent.
It was added back in 2017 by commit beef0b5 without any explanation of its intended use and it is not used anywhere else in the repository, so it's rather pointless. Remove it - it can always be added back in the future if there's a real use case.
"it's" is a contraction of "it is" - not what is intended in these cases.
|
Rebased properly on top of 1.x |
|
The std::move of trivial types is a personal choice. It doesn't happen in just that one file. I have wondered multiple times before if it wouldn't be better to remove the std::move, but I personally still find that the benefit outweights the downside. The std::move indeed does absolutely nothing. However I like it in move constructors for the same reason that I like listing all members of the class there: to quickly tell why a custom move constructor has to exist. By removing the std::move in those lines, I now need to go check the class declaration to find out whether those are trivial types or whether I simply don't want their value erased in the moved-from object. With the std::move there, I can immediately tell that I don't care about any of the values in the moved-from object (except for m_messageTopicId which is set in the body). To be clear: in any place other than the move constructor and move assignment operator I would be against the use of std::move on a trivial type. But I consider those functions to be an exception. So unless you can manage to convince me to change my mind about this subject, I'd rather not merge the "Don't std::move trivially copyable types like std::uint64_t" commit. |
|
I know the std::move of trivial types happens in many places. This was just a tester to see if you agreed with me that we shouldn't do so - you don't agree, so I'll drop that commit and ignore that in the future. :) |
BackendRenderer.hpp includes Backend.hpp which includes BackendRenderer.hpp, etc, ad infinitum. That's an include cycle which can easily become problematic, but luckily BackendRenderer.hpp does not actually need to include Backend.hpp so we can fix the cycle by just removing the unneeded include.
2 participants
Hi Texus
A small collection of small commits for you to consider :)
--
Jesper Juhl