For cpp20, renaming tests for using for scoped enums #72

celikelozdinc · 2024-01-22T13:45:11Z

Via this commit, I aimed to resolve the task using for scoped enums which is indicated on issue 58

celikelozdinc · 2024-01-22T13:48:12Z

RefactorTest/Require.h

@@ -20,7 +20,8 @@ void require_equal(char const *file, unsigned line, T expected, U actual)
    if (expected != actual)
    {
        std::ostringstream message;
-        message << file << '(' << line << "): error: expected " << expected << ", got " << actual;
+        //message << file << '(' << line << "): error: expected " << expected << ", got " << actual;


Since I've build errors on Ubuntu 22.04.3 LTS with clion, needed to change thsese lines.

The correct fix is to provide a stream insertion operator for the scoped enum.

The only place where enum class is used is in RenameCpp11.cpp and in all cases the enum is cast to an int before calling require_equal

For your new tests cases to int or move the enum to a namespace scope where a stream insertion operator can be defined would be the right fix instead of changing the entire require_equal mechanism.

I confused about this comment, on master branch, when I build refactor-test-suite, I get the errors below which states the lines that I've never changed:

.../RefactorTest/Require.h:23:65: error: call to function 'operator<<' that is neither visible in the template definition nor found by argument-dependent lookup
message << file << '(' << line << "): error: expected " << expected << ", got " << actual;
^
.../RefactorTest/RenameCpp20.cpp:73:9: note: in instantiation of function template specialization 'require_equal<std::strong_ordering, std::strong_ordering>' requested here
REQUIRE_EQUAL(std::strong_ordering::equal, x <=> x);

Ah, OK... I thought I had added stream insertion operators for those... linux CI build is compiling without errors.

What version compiler are you using?

google says ubuntu 22.04.3 LTS is using gcc-11, but I can't reproduce your error on compiler explorer:
https://godbolt.org/z/36Y6vxeTn

Please revert this change.

LegalizeAdulthood

Thanks for the contribution! Please update per my comments.

LegalizeAdulthood · 2024-01-22T16:44:09Z

RefactorTest/RenameCpp20.cpp

@@ -278,7 +278,7 @@ void f8()
    {
        // #TEST#: R937 Rename local variable vals
        // #TEST#: R938 Rename use of one
-        const int vals[3](one, 2, 3);
+        const int vals[3]{one, 2, 3};


Don't change these. The comment on line 273 explicitly says that this is initialization via parentheses.

LegalizeAdulthood · 2024-01-22T16:45:16Z

RefactorTest/RenameCpp20.cpp

+void f11()
+{
+    //#TEST#: R1521 Rename E
+    enum class E : uint8_t {


clang-format all source code changes

Thanks, clang-format is applied

LegalizeAdulthood · 2024-01-22T16:46:29Z

RefactorTest/RenameCpp20.cpp

+
+    struct F11 {
+        using enum E;
+    }f11Ins;


Separate declaration of struct and declaration of variable

Thanks, separated

LegalizeAdulthood · 2024-01-22T16:47:16Z

RefactorTest/RenameCpp20.cpp

+        using enum E;
+    }f11Ins;
+
+    const auto& get_enumerator = [&]() -> auto {


Odd that you are taking a reference to a temporary. Not undefined behavior, but just odd.

LegalizeAdulthood · 2024-01-22T16:50:03Z

results/CLionResults.md

@@ -2102,6 +2102,10 @@ R1517 |
 R1518 |
 R1519 |
 R1520 |
+R1521 |


You'll need to rebase on HEAD as I'm adding lots of tests recently. Easiest thing to do is to mark the tests as newly added and re-add them using the add-tests tool as described in Contributing.md

Thanks, rebased.

declaration&definition

LegalizeAdulthood · 2024-01-27T00:20:02Z

RefactorTest/RenameCpp20.cpp

+void f11()
+{
+    //#GOINK#: ID Rename E
+    enum class E : uint8_t


This fails to build when the enum is local to this function. I suspect that is because the anonymous class created for the lambda is emitted to the enclosing namespace scope and not as a local inside this function. If I move the enum to namespace scope, then it compiles. Please update your code to put the enum in the namespace scope to correct this build error.

Also you haven't included a header to define uint8_t and it's not important to this test, so just drop off the : uint8_t base type and declare enum class E

LegalizeAdulthood · 2024-01-27T00:22:22Z

RefactorTest/RenameCpp20.cpp

+    //#GOINK#: ID Rename E
+    enum class E : uint8_t
+    {
+        //#GOINK#: ID Rename One


Please put a space between the comment introducer (//) and the test marker

LegalizeAdulthood · 2024-01-27T00:23:04Z

RefactorTest/Require.h

@@ -20,7 +20,8 @@ void require_equal(char const *file, unsigned line, T expected, U actual)
    if (expected != actual)
    {
        std::ostringstream message;
-        message << file << '(' << line << "): error: expected " << expected << ", got " << actual;
+        //message << file << '(' << line << "): error: expected " << expected << ", got " << actual;


Please revert this change.

LegalizeAdulthood · 2024-01-27T22:58:20Z

I've corrected the problems and merged your (amended by me) commit to master.

celikelozdinc commented Jan 22, 2024

View reviewed changes

LegalizeAdulthood requested changes Jan 22, 2024

View reviewed changes

Niyazi Ozdinc Celikel added 2 commits January 22, 2024 20:47

feat : for cpp20, using for scoped enums

8b961b8

PR review LegalizeAdulthood#1 : clang-tidy, conflicting files, struct

fa691d2

declaration&definition

celikelozdinc force-pushed the feat/new_tests branch from d5d6b10 to fa691d2 Compare January 22, 2024 17:50

LegalizeAdulthood reviewed Jan 27, 2024

View reviewed changes

LegalizeAdulthood closed this Jan 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For cpp20, renaming tests for using for scoped enums #72

For cpp20, renaming tests for using for scoped enums #72

celikelozdinc commented Jan 22, 2024 •

edited

Loading

celikelozdinc Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 22, 2024 •

edited

Loading

celikelozdinc Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 27, 2024

LegalizeAdulthood left a comment

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

celikelozdinc Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

celikelozdinc Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

LegalizeAdulthood Jan 22, 2024

celikelozdinc Jan 22, 2024

LegalizeAdulthood Jan 27, 2024

LegalizeAdulthood Jan 27, 2024

LegalizeAdulthood Jan 27, 2024

LegalizeAdulthood Jan 27, 2024

LegalizeAdulthood commented Jan 27, 2024

@@ @@ -2102,6 +2102,10 @@ R1517 | @@
               R1518 |
               R1519 |
               R1520 |
+              R1521 |

For cpp20, renaming tests for using for scoped enums #72

For cpp20, renaming tests for using for scoped enums #72

Conversation

celikelozdinc commented Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LegalizeAdulthood Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LegalizeAdulthood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LegalizeAdulthood commented Jan 27, 2024

celikelozdinc commented Jan 22, 2024 •

edited

Loading

LegalizeAdulthood Jan 22, 2024 •

edited

Loading