Understanding Code You Didn't Write // Code Review
Вставка
- Опубліковано 30 лип 2024
- Keep exploring at brilliant.org/TheCherno/ Get started for free, and hurry-the first 200 people get 20% off an annual premium subscription.
Patreon ► / thecherno
Instagram ► / thecherno
Twitter ► / thecherno
Discord ► / discord
Code ► github.com/St0wy/GPR4400-Phys...
Designing a Physics Engine in 5 minutes ► • Designing a Physics En...
Send an email to chernoreview@gmail.com with your source code, a brief explanation, and what you need help with/want me to review and you could be in the next episode of my Code Review series! Also let me know if you would like to remain anonymous.
CHAPTERS
0:00 - Resuming from last time
1:20 - Reading code you didn't write
8:27 - Easily find the entry point
10:51 - How the program works
15:32 - Code should be self-documenting ✔
17:14 - Comments in code to control tools ❌
17:55 - static_cast and dynamic_cast
20:19 - structs vs classes: when to use what
24:19 - Why prefixing member variables is useful
This video is sponsored by Brilliant.
#CodeReview
This is Part 2! Watch Part 1 here: ua-cam.com/video/eNSkYAzC_ew/v-deo.html
Keep exploring at brilliant.org/TheCherno/ Get started for free, and hurry-the first 200 people get 20% off an annual premium subscription.
Do you accept java code for the code review series?
@@lengors7327 You definitely need some love if you really do program on Java.
I dont code much so I have no idea what you're all talking about, but Stowy is my friend and I'm very proud of him and his work being celebrated and featured here
That's a solid friend move. Good looking out!
Thanks a lot for looking at my code again ! These feedbacks and tips definetly where super usefull ! (although i'm very nervous that so many people sees it haha)
I'll try to fix the problem with the header files and the wrong usage of structs tonight, so if you want you can pull my changes before an (hypothetical ?) part 3.
Just pointing out:
Your full name and email are in the video, are you okay with that? 😅
@@Thebreak1 Didn't really think about it, but yeah it's okay, it was already public on github before that haha
@@caoinismyname yeah i see what you mean, but this is also kinda the rule I tried to follow, even in C# for example. Just that when I followed the tutorial form Winterdev, classes and structs where kinda mixed up together and I was scared to change it in case it was for a valid reason later in the tutorial.
@@Stowy Well in C# you cant have an empty constructor for a struct. One thing I was annoyed of, because I needed an empty one to store 4 different variables that also know if they were assigned or not.
@@Stowy While you're learning is the best time to make changes, even if you are scared it will mess things up. Sometimes changing something helps to better understand what you're learning. If it does mess up later and you can change it back then you truly understand the logic of the code and aren't just parroting the tutorial.
Understanding Code You Didn't Write. Next day realizing it still was me.
Don't comment your code and you'll have this experience every time you look at something you wrote more than three weeks ago!
@@cyqry git blame
@@muumitramm That's a new one for me and I love the name, like they know *exactly* why you would use that.
@@muumitramm Yeah, but it's always DNS.
If (x != y && x
C++ style casts are encouraged because they make explicit exactly which casting behavior is intended. There are inherent benefits to using them over C-Style casts, especially on projects which have multiple developers touching the code base.
There are many cases in which complicated runtime casting can be vague if you are not explicit such as: (std::byte*)&integers[index] where integers is some container like an std::array. The above is NOT a static cast and is a reinterpret cast which can be a very error prone cast if you don't know exactly what you are doing. The benefit of explicitly using reinterpret_cast(&integers[offset]), is that you are "self-documenting". You might see such casting in low level optimizations when you're working on something like Serialization code to translate a variety of data types into and from a block of raw memory.
This tells anyone looking at your code: hey this is special cast we are treating the value at this memory address as a different data type that it normally is not meant to be treated as! Especially if you work on a large code base where you may have multiple authors potentially contributing to code this sort of explicit casting is preferable, because it is less error prone and tells other people "pay attention to what is actually going on here and if you don't know what this is explicitly doing go look it up". This can help prevent bugs being introduced by some other contributing developer.
Similar cases are common with the use of dynamic_cast. We want to clearly express "Hey we know because of some condition check or other control flow that this variable should be a derived type here at this point in the code." It also makes clear to any other developer where potential performance bottlenecks might be located at because they can see this is a run time polymorphic cast, if we are doing a lot of function calls off this we are probably doing a lot of vtable lookups here in this area.
Thirdly, searching through a codebase for "dynamic_cast
In terms of the ranged for loops, I prefer to use your second solution and make the key variable to be just an underscore to specify that it wasn't used as I do agree with the video that the | std::views code makes the code really unreadable.
+1 for self documenting code! Comments can lie when not updated but code never lies.
Code can lie if you don't run it. That's why I'm in the habit of removing dead code wherever I can find it.
Thanks for the shout out! When I first moved from C# to C++ my first project was to make a game and I used your tutorials to create the first versions of the engine. If you look into the repo you can see traces of your layer model :) I don't know why that physics engine video blew up, but it's awesome to see it come full circle!
Thanks a lot for the tutorial, it helped me so much to have a good grade for this project haha
@@Stowy Nice! I've mined the physics engine knowledge for many As throughout the years haha
@@WinterdevUnless there's a part 3, I'll be watching your tutorial next.
It should be good for helping me understand physics engines better, even if I use a framework!
Absolutely agree on the Struct theme.
For me structs are data, maybe with functions to accept ony specific data, but still data.
classes on the other hande are there to manipulate data.
So in my code I can clearly distinguish between the the two and easily identify whether or not data could be changed by it.
same here
Definitely a 3rd part needed. I just love this guys code quality 😍
THAT'S THE SMOOTHEST TRANSITION INTO A SPONSOR THAT I HAVE SEEN!! DAMN
If not mistaken for the headers to show up in vs projects as source when using cmake, the header files need to be added to the target as a "source"; I.e. add_library(target header.hpp)
Ah yes I haven't done that and I always use the folder view so I didn't notice this problem, thanks for the solution !
"Understanding code you didn't write!" My biggest gripe when dumped with a project someone else abandoned. And then you go on a call and seniors suddenly think you know how it all works. Grrrrrrrrr
The rules for the underscores are not different per compiler. They are indeed standardized, and they are the same for C and C++, for header file compatibility. In C11, this is all defined in §7.1.3. And it reads: "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces." In this case, having member variables with one underscore and a *lowercase* letter is OK, because those identifiers are not on file scope. Change it to uppercase and you fall into the first category, where it is always reserved.
The reason it is always reserved is because the implementation may end up having to define internal macros to do its job, and if you use the same name, you are going to have a bad time.
I believe that having a single underscore and then a capital letter is prohibited by the standard, otherwise it's implementation dependent. Microsoft reserves double underscores, but also utilizes the _A pattern (std::array for instance has its only member called _M_elems afaik, which allows for some magic)
Yes, the C++ standard actually reserves single underscore variables for itself, while MS reserves the double. I don't think it matters if it's capitalized after that or not. But I've not read the standard that closely in over a decade haha.
@@crazycdn8327 yep, I agree, though I've seen something like that in C (probably when reading K&R, or cppreference)
@@crazycdn8327 the C/C++ standard reserves multiple things, including functions which start with either "str" or "mem".
The important ones are identifiers starting with underscore and then a capital letter (like _Generic, or _Atomic), while double-underscores at the start are reserved for standard library implementations.
Clang and GCC use "__builtin_" at the start of every built-in keyword used to implement a language feature (e.g "offsetof()" is implemented as "#define offsetof(t, d) __builtin_offsetof(t, d)"), but MSVC uses just a double underscore.
I tend to agree with the separation between struct and class. When i think struct it's something that gets operated on like a data STRUCTure. Classes on the other and are the operators that work on the structures.
This is really great I didn't think about actually running the code in debug mode and watching it line by line.
Being pedantic, force = gravityForce * mass looks wrong unless gravityForce is really acceleration due to gravity thus satisfying the basic kinematic equation F = ma
well gravityForce is a variable that dynamic bodies have that's applied if "usesGravity" (or something like that) is true. So basically it's 9.81. Should this be called gravityAcceleration to be more exact ?
@@Stowy absolutely, this an acceleration not a force.
Lots more to talk about. May as well hang on to this for as long as you can keep pulling out interesting topics you otherwise may not have covered! Top work
I agree with you on the struct vs class. The class keyword hints that C++ happens here where a struct would normally be a POD or something that allows passing to C code
@2:38 Header files are compiled. They may be compiled multiple times and the reason why your linker doesn't freak out and start going all one definition rule on you is because they are exactly the same information.
@5:00 Stepping is the way to go. In OOP languages you can get burned ctrl+click'ing through projects because the IDE may not be smart enough to find what functions are being overridden especially when dependency inversion with interfaces is involved. Super annoying but part of the experience. Also note that in C++ when looking through a desktop application, main can also take a 3rd argument that shows the environment variables.
"A very common implementation-defined form of main() has a third argument (in addition to argc and argv), of type char**, pointing at an array of pointers to the execution environment variables.'
This is the exact reason I came upon your youtube channel. Trying to figure out my co-worker's 20 years old C++ code.
thank you for this great content, every video inspires me to rethink my projects and work on good habits while coding :). Is there a vidoe where you talk a bit about your history and how you got to the point you are at now? Would really be interested in that!
yay new code review vid
I would really like to see you actually implement these changes in a future video! I think it would be very instructive. Thanks for the insights you've already provided.
LOVE YOU! THANK YOU FOR ALL YOUR VIDEOS!!!!! Greatings from Turkey.
Part 3 is very much awaited!
I've been watching these 2 episodes thinking "Man this is all so interesting, but the poor lad hasn't got their code reviewed yet 😅".
As answer for the question at the end: I do enjoy just watching you talk about code :)
19:20 It is usually a good idea to add a virtual destructor to any class that is intended to be a base class. Otherwise you may end up in some bad situations where destructors and not called properly. As a side effect, this will allow dynamic_cast to function (assuming RTTI is enabled) because there is now a vtable. As a side note, RTTI adds overhead that is generally not desirable in game engine code, so it is often disabled and dynamic_cast will not function anyway.
The tricks of stepping through the debugger was really helpful :)
It lets me understand more why some people defend it over just printing things to the console :).
Debugging by printing is more difficult, and should be last resort when you don't have at least mediocre debugger, or you already exhausted debugger to the limits and the bug was not located.
The reason why printing is more difficult is, that debugger actions are mostly transparent to the debugged code, while printing to console is being run as part of the debugged code, thus it may affect current state lot more easily than a debugger. You can by accident modify some variable by doing stuff like `items++` in the print statement, or you can modify timings of critical sections enough to make the code magically work when having logging enabled, but getting some crash due to multihreaded race condition when the code is running at top speed without logging. Etc.
Using debugger can still slightly affect the state of the code (especially timing-critical bugs like race conditions between different threads), but at least you can't accidentally modify some variable while you were trying just to print it to console.
In the end, one should be able to do both, as sometimes logging is more helpful, but default choice should be debugger, always (or unit tests and avoiding any debugging completely :P ).
I like to read code with a modular approach where I isolate the different modules and try to figure out what they do on their own. It's a lot easier than trying to trace the execution. That's just my opinion though, definitely use a debugger!
You've just answered some questions I've recently had
you are the coolest programmer ever. and i like the way you talk , thanks for teaching me so much
what is the font and color theme in his viusal studio? I would be very grateful if someone tell me.
If I may know, what extensions do you use Cherno for your visual studio?
4:29 How can I get those tools in Visual Studio Code?
Your videos are amazing and i'll glad if you make a video on ''How to make window in C++"
Just waiting for this topic
you won't need to wait if you type in the search bar
Towards the end with the naming convention of variables, I've never had any issues with using a single leading underscore as long as it is a member of a class or a struct. As for globals variables or even standalone variables within a namespace I avoid using them.
_Technically _this _is _not _a _problem. _It _is _just _misleading _in _plain _English _and _very _disturbing _in _code ;-)
Hey @TheCherno I'm running through your game engine series as of rn and want to make it like hazel but with the physics of like snowdrop from before they downgraded it any advice ^^
really enjoying the series
Tim's santa hat in the background is the best
Interesting new zoom effects. I can almost hear "Whoosh!" whenever they play.
Hi Cherno, if you read this, it would be extremely useful and appreciated if you could show "IDE agnostic" debuggers for C++, specially in Linux. Visual Studio is no always available so getting good debuggers is tricky.
Thanks!
CLion has the best debugging experience on Linux IMO
@@SigSeg-V Well, not paid 😅...thanks though!
I don't use linux but the two I hear about most often are GNU debugger and LLDB. VS Code is also available on Linux but it's more of a code editor.
@@Spero_Hawk I hate GNU Debugger, will give LLDB a try. VS Code has an extension for debugging, but it's really not that great. Thanks though!
Very true
Been writing game engines professuonally for almost 30 years...have myy own as a side project. Would be interesting to hear your thoughts but it's way large (multiple 100k written by me) and I am doing everything including deleting or implementing copy and move constructiors.
Currently working on an OS level GUI abstractions layer to assist with writing tooling. Lol I wrote a layer over the old win32 C api in c++, but I also wrote a qt implentation
Moving the program counter can also corrupt your stack if you move it in/out of the stack, like out of a loop etc.
Thanks
Very interesting about the difference between class and struct in C++. I haven't written much C++ in quite a few years but I know that some other languages like C# have a lot more differences between class and struct which might be where the confusion comes from. It's also quite common in C# to use the _privateMember naming convention.
@@jeffmccloud905 well in c++ the only difference is the public by default in structs, the differences in c# are way more fundamentals. The fact that structs are allocated on the stack and other syntactics differences make them very different than classes compared to c++
@@jeffmccloud905 bruh allocation on the stack and the heap is done trough the new keyword or smart pointers in cpp. For ex the first line is on the stack and the second one is on the heap:
Vector2 v = Vector2(1, 1) // Stack
Vector2* v = new Vector2(1, 1) // Heap
And this is the same whether vector2 is a struct or a class. Structs in cpp can be seen as just there for retro-compatibility with C.
@@jeffmccloud905 yeah it's true that in usage it's pretty rare since it's usually used to represent data, and usually pretty small amounts of it, and it's actually quite confusing with the other similar languages, that's an easy mistake to make so no worries haha
@@jeffmccloud905 The "garbage collector" is the big difference. It's actually somewhat a myth to say that structs are always allocated on the stack in C#. That's not always the case and it would be more correct to say that value types are stored on the stack in the Microsoft implementation of the desktop CLR under certain circumstances. There's actually plenty of reasons why your struct might end up on the heap in C#.
But more importantly, the only time when any of this matters is when you're trying to prevent the garbage collector from kicking in the middle of the game loop and cause framerate hiccups. Since you don't have a lot of control over when the garbage collector kicks in directly, what you end up spending time on is preventing unwanted "garbage" from being created in the first place. In other words, you care about how memory is being allocated so that it won't be deallocated when you don't want it to be.
I mostly agree with your definition of struct, but more exactly, in my view a struct doesn’t encapsulate anything, so it doesn’t hide or protect any of its members, simply group them. It can have methods, but those methods need to work with all possible values assigned to the fields of the struct (maybe excluding null pointers) and they have to be just like functions, so basically be rewritable into something like void function(T x) {} instead of a method on T. And no inheritance (because I think that’s still theoretically possible?)
cherno is the best
This would have been so hopeful a week ago XD
Do you have good techniques to understand bash code ? Bash code can't be debugged like C++.
Header files are separated because this project is a library. Includes folder just get copied at "installation" step
16:20 totally agree. If you leave a comment in code, it should tell "why" is something implemented, not "how" it works because the implementation should be readable from code as much as possible. Also thank you for mentioning the nonsense that are short variable names which don't explain their meaning. It just doesn't make sense to shorten variable names to single letter variables, there is really no benefit to it. Yeah congratulations you just saved yourself from typing few characters, but reading that code later nobody will be grateful for that.
can't wait for when my modified windows kernel actually builds and i can do this stuff to understand windows
true
Hahahaha I'll immediatly goto the last video because I want to hear about the memory
ily (:
Demo BALLS and cubes, why not Demo spheres and cubes 🤔
funny thing is, they're circles and squares
personally I just make structs simple and classes complex, but if structs to everything classes do and they're also backwards compatible with C
doesn't that just make them the superior tool?
why was class made if it's the same thing?
I agree that code should mostly be self documenting, but when it comes to math you really need comments no matter what names you give your variables. Pretty sure it's in the standard that identifiers with leading underscores are reserved. Kind of agree about struct versus class, but I think "rigid body" should be a POD. Of course, if you want truly self documenting code you should never use auto.
16:45 lmao try saying that to linux developers
don't worry, my friend! they're just saving memory by using less letters!
"Hedaphile" - The Cherno
3rd
1st!
Next up: how to write code that's readable to other people :-)
679th!
Suprised this isn't sponsored by Visual Studio.
you look like MrBeast's long lost brother
15:52 A С++ code review guy honesly hates one of the most demanded feature in c++20...
At this exact place personally I would say:
- you should add an additional std::views::filter to loop only through dynamic bodies, instead of that first line check inside of the loop body.
- add 'using name std;' to drop all these redundant std:: qualifiers.
- don't use 'auto' keyword when declaring a subject of a loop. As a reader of the code, I dont know the type of the values of _bodies. You hid it from me with the 'auto' keyword for no apparent reason.
I didn't know about the std::views::filter, most of that c++20 stuff is from resharper hints tbh, i'll see how that works 🤔
for using namespace std, there's a cherno video where he explains why it's not the best, especially in header files. I personnaly got used to it
I see why you would say that, i like the word auto in general, and in the context of this project i think it's not too bad, but i can definetly see how it can get confusing on a bigger scale
thanks for also giving feedback :D
@@Stowy The new ranges library has plenty of useful stuff. And don't take everything what Cherno says as a c++ world rule. He's stuck in the past. He can't accept even the simplest new little things like range views. Looking at how he reacts on views::values, I can only try to imagine his reaction on asynchronous code design with c++20 coroutines. It will blow his mind completely out of his head.
@@sheeftz well i do think the syntax for ranges is kinda weird, although it's super usefull, i can see how when you're not used to it, it can look very wrong
@@sheeftz oh thanks, i'll check it out !
@@sheeftz why did you recommend using namespace std?
It's true that I also suddenly talk about memory when I have conversations with friends.
man I missed this kind of tutorials lol. Great work here, thanks!!!
thank you so much , it worked