Open
Conversation
The C/C++ extension has a setting that allows you to check for different versions of C/C++. This has been set to our project defaults in order to ensure that the project standards are being acknowledged by VSCode.
CmakeLists.txt now has defaults set for the C version and C extensions used in the project. Also, there is a new flag, GRAIL_TIDY, that runs clang-tidy checks during the compile process.
After running clang-tidy during the compile process, it became apparent that Benchmark.hh and testfmt.cc both had several constexpr violations (trying to create non-constexpr objects in constexpr functions, calling non-constexpr functions in constexpr functions, etc.). These should all be fixed now.
clang-tidy has a set of reasonable defaults that should be useful for preliminary static analysis. Once we have cleared up some of the issues in the codebase, we should consider using the modern checks.
Previously, SocketIO was a class containing static functions and constants. In a situation like this, it makes a bit more sense to use a namespace (eventually if at some point we want to allow argument dependent lookup for a function).
Made some changes suggested by clang-tidy in IPV4Socket
The CPPCoreGuidelines are an open-source list of suggestions for safety and simplicity in the latest standards (currently C++ 11, 14, and 17) led by Bjarne Stroustrup. clang-tidy happens to have a set of checks to look for compliance with these guidelines, so they have been turned on for the time being.
DynArray now has emplace_back and push_back functionality. While DynArray::push_back seems to act the same as DynArray::add, DynArray::emplace_back uses an implementation of std::construct_at written in DynArray and std::forward to construct the object in-place in the DynArray. testDynArrayPerf currently has several test checking the performance of adding or emplacing raw pointers vs unique_ptrs. More testing is needed. As of right now, it looks like a DynArray of unique_ptrs has roughly the same performance as a DynArray of raw pointers (besides the initial overhead in the constructor, but that is unavoidable).
SolarSystem has some imports removed and now moves copies of names instead of passing by reference. Constructor is now explicit to prevent accidental implicit conversion. TestDrawBlockMap now implements move constructor and move operator=.
TODO: consider making callbacks a namespace. Everything in there seems static anyway.
clang-tidy was complaining about a non-virtual public destructor, but fixing that required implementing a destructor. After some more bargaining with clang-tidy, I had to implement the big five.
SocketIO explicitly declared err_code as static, but this is already implicity done due to err_code being in an anonymous namespace. The explicit use of static has been removed. SocketIO send and recv have new type signatures. Each function now returns a ssize_t to be in line with Linux's function signatures. Given that the functions originally returned an int. No functionality should change. SocketIO recv used to take in a const char* buffer and cast to a char*. Given that recv does modify the buffer, the buffer is now explicitly a char* and then will be either kept a char* (on Windows) or implicitly cast to a void* (on Linux). Given that void* and char* have the same size, this should not present any issues. SocketIO send and recv now use the flags provided. Previously, each function passed 0 as the only flag. Be aware that Grail is designed to be cross-platform and, as such, any flags used should be avaiable on Windows or Linux.
XDLRequest had an unnecessary import which was removed. The function buildData is also commented out, as it is the base case for a function which is also commented out. GLWinFonts had a few loops where the counter is guarenteed to never be negative, so int was replaced with uint32_t. Callbacks had a static void function that was never given a void type when it was declared. The implementation gave the correct type, but the declaration needed to be fixed.
Currently, there are many places where an if-statement with only one line following it is used without providing any braces. This is primarily done out of convenience, however it also means many flags by clang-tidy for readability issues. We solved this with indentation, however if it ever becomes an issue we can always revert the setting so any if-statement without braces triggers a warning.
DynArray has several spots it could use uint32_t and was instead using int. This has been fixed. Adds in a comment asking for feedback on what operator new does. It seems to just return one of the parameters without doing anything. Converts DynArray::find(T&) to a const function, as contents of the array nor the key are ever modified. Adds comments that should be recognized by clang-tidy to remove any warnings about pointer arithmetic. This is a container implementation and some pointer arithmetic seems unavoidable.
There are several modifications that were made for compatbility reasons, however these changes are all several months old by now. These changes are being added in all at once. Fixes will be made as the CI/CD system reports errors. AxisWidget: Include cstring to provide strlen MultiText/MultiText2: Remove functions that have "unsigned long long" as a parameter. Using functions with byte-specific types end up being better for cross-platform compatbility. Benchmark: Remove sys/times include. MSYS does not provide this and we do not need it anymore. DynArray: Add DynArray<T>::removeAt(uint32_t index) -> T. This removes a particular element from the array and returns it. Warning: This currently achieves this by copying the object to a temp variable, sliding all other elements forward by one (without moving?), and then returning the copy. For some reason, this invokes the original object's destructor if its the last element in the array? I believe this needs to be rewritten a bit before it is ready for use. testDynArray: This runs through a couple test cases for DynArrays (adding and removing integers, strings, and Tabs). This was originally done as a way to test how adding and removing tabs worked out, but could be useful in the future. This should eventually turn into a compile-time test using static_assert, as DynArray should be able to be made into an entirely constexpr class. testfmt: Removes constexpr specifier on functions that call sprintf. constexpr cannot be used on non-constexpr functions and sprintf is not constexpr.
I think these changes should have already been added in a previous commit. Hopefully there is nothing funky going on here, but I'll investigate the commit history after the merge is completed.
…nto tabbed-windows
CMake checks the hash of several json files during configuration and runs a python script for that json file if it has been updated since the last configuration. The messages emitted from this code generation step had an extra newline at the end. This has been removed to keep all CMake messages uniformly spaced
testfmt: sprintf is a C function and therefore cannot be used in a constexpr function. As such, the functions using sprintf are no longer marked as constexpr.
Build CI depends on the name of the branch, which has been changed since the last commit. This should update that information.
tabbed_windows is now a part of a PR, so every test is getting run twice. This has been fixed to only have one test set running per commit.
…nto tabbed-windows
Ubuntu 18.04 cannot use video at the moment due to a dependency bug, so MPV should be removed to prevent building video components. A more permanent fix should be implemented for making video optional.
The converter is not 100% complete yet and does not currently build. Once this is fixed, it can be added back in.
Signed-off-by: ahuston-0 <aliceghuston@gmail.com>
Signed-off-by: ahuston-0 <aliceghuston@gmail.com>
Signed-off-by: ahuston-0 <aliceghuston@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I thought Tabbed Windows was synchronized but there are a few tests that apparently never got merged in. I'll review this tonight and get it merged in.