Skip to content

Updates to copypastemanager#390

Merged
danbelcher-MSFT merged 18 commits intomicrosoft:masterfrom
Nicholas-Baron:updates-to-copypastemanager
Apr 22, 2019
Merged

Updates to copypastemanager#390
danbelcher-MSFT merged 18 commits intomicrosoft:masterfrom
Nicholas-Baron:updates-to-copypastemanager

Conversation

@Nicholas-Baron
Copy link
Copy Markdown
Contributor

@Nicholas-Baron Nicholas-Baron commented Mar 26, 2019

Fixes #55 and #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

Nicholas Baron added 4 commits March 25, 2019 22:33
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
@msftclas
Copy link
Copy Markdown

msftclas commented Mar 26, 2019

CLA assistant check
All CLA requirements met.

@Nicholas-Baron
Copy link
Copy Markdown
Contributor Author

@rudyhuyn The changes that were suggested are now in the code.

@grochocki grochocki added codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) fixing approved issue labels Apr 3, 2019
Copy link
Copy Markdown

@danbelcher-MSFT danbelcher-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should all be inline constexpr

Copy link
Copy Markdown
Contributor Author

@Nicholas-Baron Nicholas-Baron Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these instead be static?
C++17 static constexpr implies inline.
Source: cppreference

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++

Copy link
Copy Markdown
Contributor Author

@Nicholas-Baron Nicholas-Baron Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Daniel Belcher and others added 2 commits April 21, 2019 21:54
Removed an unneeded `inline`

Co-Authored-By: Nicholas-Baron <[email protected]>
Removed an unneeded inline.

Co-Authored-By: Nicholas-Baron <[email protected]>
Copy link
Copy Markdown

@danbelcher-MSFT danbelcher-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Nicholas-Baron, thanks for being patient through the review process!

@danbelcher-MSFT danbelcher-MSFT merged commit cdd2b1c into microsoft:master Apr 22, 2019
EriWong pushed a commit to EriWong/calculator that referenced this pull request Jun 5, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) fixing approved issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Calculator should use constexpr when defining static const strings

5 participants