Updates to copypastemanager#390
Updates to copypastemanager#390danbelcher-MSFT merged 18 commits intomicrosoft:masterfrom Nicholas-Baron:updates-to-copypastemanager
Conversation
Noticed that some wstrings (i.e.: those that use `+` ) cannot be constexpr-ed. Added other two autos and one const in the same file.
Added some auto deductions. One C-cast changed to a static_cast.
Removed an unused value
Optimized one loop, added a default catch
|
@rudyhuyn The changes that were suggested are now in the code. |
danbelcher-MSFT
left a comment
There was a problem hiding this comment.
Hey @Nicholas-Baron, I just have a few comments building on top of your changes. Feel free to discuss if you disagree with any of them or if you have questions. Thanks!
| #define OctBase 7 | ||
| #define BinBase 8 | ||
| // TODO: Could be made into an enum : unsigned ? | ||
| constexpr auto QwordType = 1; |
There was a problem hiding this comment.
These should all be inline constexpr
There was a problem hiding this comment.
Could these instead be static?
C++17 static constexpr implies inline.
Source: cppreference
There was a problem hiding this comment.
Are you referring to:
A constexpr specifier used in a function or static member variable (since C++17) declaration implies inline.
? The use here doesn't fit either case. We need to look at the documentation for storage duration. Here are the relevant parts, shortened for brevity:
internal linkage. The name can be referred to from all scopes in the current translation unit.
Any of the following names declared at namespace scope have internal linkage:
- variables, functions, or function templates declared static;
- non-volatile non-inline (since C++17) const-qualified variables (including constexpr) that aren't declared extern and aren't previously declared to have external linkage;
The second bullet point describes the proposed change (constexpr auto QwordType = 1;). This variable has internal linkage, meaning each translation unit has its own copy of the definition. The first bullet point describes the addition you suggested (static constexpr auto QwordType = 1;). The variable already has internal linkage so there is no difference between the two.
The goal for the codebase is that there is 1 definition for these expressions and so the appropriate keyword is inline. This will force all references to the variable across translation units to actually refer to the same definition. We can test all of this with a simple program that prints the address of these expressions in two different translation units. I left out some of the headers just to keep the example simple.
// Common.h
constexpr auto var_constexpr = 1;
static constexpr auto var_static_constexpr = 2;
inline constexpr auto var_inline_constexpr = 3;
// Source1.cpp
void Source1::print()
{
cout << "Source1::print\n";
cout << "\tconstexpr: " << &var_constexpr << "\n";
cout << "\tinline constexpr: " << &var_inline_constexpr << "\n";
cout << "\tstatic constexpr: " << &var_static_constexpr << "\n";
cout << endl;
}
// Source2.cpp
// same as above
// Main.cpp
int main()
{
Source1::print();
Source2::print();
}Source1::print
constexpr: 00329B30
static constexpr: 00329B34
inline constexpr: 00329B3C
Source2::print
constexpr: 00329B98
static constexpr: 00329B9C
inline constexpr: 00329B3C
As discussed, only inline constexpr forced the single definition across the program. This explanation was rather long, but the topic comes up often and I have a sample program I keep on hand :) Linkage is a common source of confusion in C++
There was a problem hiding this comment.
Yeah, I have heard that linkers are not as frequently updated as other parts of a compiler.
Question about member variables: what would be the difference between static constexpr and inline static constexpr?
There was a problem hiding this comment.
A member variable that is static constexpr is implicitly inline, therefore, there would be no difference. inline means that "multiple definitions are permitted". Imagine that it were not the case that static constexpr implies inline for a member variable. Then consider:
// Class.h
class Foo {
static constexpr bar = 1;
};
// Source1.cpp
#include "Class.h"
// Source2.cpp
#include "Class.h"Each cpp is its own translation unit and, therefore, each provides a definition for bar. Without inline, implied or explicit, then the duplicate definition would be a violation of the One Definition Rule.
According to inline, "A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function.". This case is describing:
class Foo2 {
void print() { std::cout << "hello!"; }
};As with the scenario above, if the member function definition was included into multiple translation units, there would be an ODR-violation, therefore member function definitions are implicitly inline as well.
Removed an unneeded catch
Removed an unneeded `inline` Co-Authored-By: Nicholas-Baron <[email protected]>
Removed an unneeded inline. Co-Authored-By: Nicholas-Baron <[email protected]>
danbelcher-MSFT
left a comment
There was a problem hiding this comment.
Hey @Nicholas-Baron, thanks for being patient through the review process!
Related to microsoft#55 and microsoft#64 Description of the changes: Added constexpr to formerly static const or #define variables Applied C++ Core Guideline NR.2 Added auto and const in appropriate places How changes were validated: Used the provided unit tests
Fixes #55 and #64
Description of the changes:
constexprto formerlystatic constor#definevariablesautoandconstin appropriate placesHow changes were validated: