As reported leper in rP19703, when compiling with clang we get some warnings. Most importantly the undefined behavior when shifting 8 bits of a byte type.
Details
- Reviewers
fcxSanya - Commits
- rP20582: Fix two clang warnings in rP19703 reported by leper.
I quickly tested that STUN connections still succeed. But please make sure that I didn't change the intended behavior of the byte getter.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 3915 Build 6837: Vulcan Build (Windows) Jenkins Build 6836: Vulcan Build Jenkins Build 6835: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
source/lobby/glooxwrapper/glooxwrapper.h | ||
---|---|---|
641 ↗ | (On Diff #4364) | This seems strange. I suspect not actually handling this properlymight make make us leak memory (to be more specific m_Wrapped). |
source/network/StunClient.cpp | ||
127 | I guess one could provide a specialization of the other function so someone calling that with u8 does not run into the same issue again. |
You are right about a specialization (I'm not too familiar with templates yet).
source/lobby/glooxwrapper/glooxwrapper.h | ||
---|---|---|
641 ↗ | (On Diff #4364) | Perhaps nicer in a separate memory leak investigation patch if we can fix the other two things beforehand |
source/network/StunClient.cpp | ||
127 | Since n is a deduced argument, it could only be template<u8> bool GetFromBuffer(const std::vector<u8>& buffer, u32& offset, u8& result) But then I still have the clang compile warning. Since this function copies one line, we can just keep that one function and add that n=1 special case to that function if we can't resolve the specialization however. I believe it might be possible to do something with template constraints http://en.cppreference.com/w/cpp/language/constraints (search for size on that page), but that looks alien and might even end up more complicated and longer. |
Successful build - Chance fights ever on the side of the prudent.
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
These 3 methods (AddUInt16, AddUInt32, GetFromBuffer) are copied from STK's BareNetworkString. I didn't copy the entire class since I tried to extract the necessary code only, hoping to replace these general methods with pyrogenesis' own or library counterparts.
I think it would be confusing to call GetFromBuffer with non-empty result param and n different from the result size in any case (f.e. reading one more byte into uint16), so the first shift (when a = n - 1) shouldn't be needed.
@fcxSanya you mean replacing if (n > 1) with if (a < n - 1) check so that the very first time entering the loop we skip the bitshift, thus fix the issue for the byte type and also preventing any useless bitshift beyond the type size for any other type?
Sounds preferable to me because we can delete the comment then.
I just said that either your uint8-specific if (n > 1) or a more generic if (a < n - 1) should be fine given the method expected usage.
If you will replace the check, wouldn't you still get the compiler warning? (I'm not sure how intelligent the compiler is in calculating those)