selectCaptureGroup will work with named capture groups by Delwing · Pull Request #5131 · Mudlet/Mudlet

Delwing · 2021-04-10T12:08:14Z

Brief overview of PR changes/additions

Make it possible to use selectCaptureGroup with name parameter

Motivation for adding to Mudlet

Consistency.

Other info (issues closed, discussion etc)

closes #5107

Release post highlight

add-deployment-links · 2021-04-10T12:08:15Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

github-actions

clang-tidy made some suggestions

src/TLuaInterpreter.cpp

github-actions · 2021-04-10T12:16:14Z

src/TTrigger.cpp

            int n = (tabptr[0] << 8) | tabptr[1];
-            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed();
-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000")); ^

github-actions · 2021-04-10T12:16:15Z

src/TTrigger.cpp

-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));
+            qDebug() << name;
+            char* substring_start = subject + ovector[2*n];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

char* substring_start = subject + ovector[2*n]; ^

github-actions · 2021-04-10T12:16:15Z

src/TTrigger.cpp

-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));
+            qDebug() << name;
+            char* substring_start = subject + ovector[2*n];


warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]

char* substring_start = subject + ovector[2*n]; ^

github-actions · 2021-04-10T12:16:15Z

src/TTrigger.cpp

+            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));
+            qDebug() << name;
+            char* substring_start = subject + ovector[2*n];
+            int substring_length = ovector[2*n+1] - ovector[2*n];


warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]

int substring_length = ovector[2*n+1] - ovector[2*n]; ^

github-actions · 2021-04-10T12:16:15Z

src/TTrigger.cpp

+            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));
+            qDebug() << name;
+            char* substring_start = subject + ovector[2*n];
+            int substring_length = ovector[2*n+1] - ovector[2*n];


warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]

int substring_length = ovector[2*n+1] - ovector[2*n]; ^

vadi2 · 2021-04-10T12:34:25Z

src/TTrigger.cpp

            int n = (tabptr[0] << 8) | tabptr[1];
-            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed();
-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));


Can you add a comment for the remove()? It's purpose not immediately clear.

Sure, just added.
it does not propagate to Lua, but I've noticed that this null is somehow added sometimes to name of capture group.
Might be better way to deal with it - but I don't know it - this for sue helps.

You are dealing with data from a C library and it's outputs are going to be \0 terminated const char[] C strings. The name_entry_size value is going to be a strìng length argument and if you are trying to return partial string extraction with the original strings you should be referring to it (I believe) with a &tabptr[2] rather than tabptr + 2. Indeed given that as a first argument to the QString::fromUtf8(...) method I believe it is smart enough to remove just the first 2 characters from whatever it is that the original tabptr pointed to without having to specify an explicit length of string to consider as a second argument (and thereby not needing the QString::remove(...) either...

remove remark - I've already managed to figure that one out, by removing second argument all was fixed and remove could be deleted.
I've also just changed first argument as suggested.

github-actions

clang-tidy made some suggestions

github-actions · 2021-04-10T12:36:24Z

src/TTrigger.cpp

            int n = (tabptr[0] << 8) | tabptr[1];
-            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed();
-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000"));


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

auto name = QString::fromUtf8(tabptr + 2, name_entry_size - 3).trimmed().remove(QStringLiteral("\u0000")); ^

vadi2

Nice improvement! A bit more polish needed - it seems to get confused about the captures:

test-trig.zip

src/TTrigger.cpp

src/TLuaInterpreter.cpp

Delwing · 2021-04-12T15:26:16Z

Sure, will do corrections.

github-actions

clang-tidy made some suggestions

github-actions · 2021-04-15T16:30:44Z

src/TTrigger.cpp

        }
    }
+
+    int namecount;


warning: variable namecount is not initialized [cppcoreguidelines-init-variables]

int namecount; ^ = 0

github-actions · 2021-04-15T16:30:44Z

src/TTrigger.cpp

    }
+
+    int namecount;
+    int name_entry_size;


warning: variable name_entry_size is not initialized [cppcoreguidelines-init-variables]

int name_entry_size; ^ = 0

github-actions · 2021-04-15T16:30:44Z

src/TTrigger.cpp

+
+    int namecount;
+    int name_entry_size;
+    char* tabptr;


warning: variable tabptr is not initialized [cppcoreguidelines-init-variables]

char* tabptr; ^ = nullptr

src/TTrigger.cpp

github-actions · 2021-04-15T16:30:45Z

src/TTrigger.cpp

-            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed();
-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(&tabptr[2]).trimmed();
+            auto* substring_start = subject + ovector[2*n];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

auto* substring_start = subject + ovector[2*n]; ^

github-actions · 2021-04-15T16:30:45Z

src/TTrigger.cpp

-            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed();
-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(&tabptr[2]).trimmed();
+            auto* substring_start = subject + ovector[2*n];


warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]

auto* substring_start = subject + ovector[2*n]; ^

github-actions · 2021-04-15T16:30:45Z

src/TTrigger.cpp

-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(&tabptr[2]).trimmed();
+            auto* substring_start = subject + ovector[2*n];
+            auto substring_length = ovector[2*n+1] - ovector[2*n];


warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]

auto substring_length = ovector[2*n+1] - ovector[2*n]; ^

github-actions · 2021-04-15T16:30:45Z

src/TTrigger.cpp

-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(&tabptr[2]).trimmed();
+            auto* substring_start = subject + ovector[2*n];
+            auto substring_length = ovector[2*n+1] - ovector[2*n];


warning: do not use array subscript when the index is not an integer constant expression; use gsl::at() instead [cppcoreguidelines-pro-bounds-constant-array-index]

auto substring_length = ovector[2*n+1] - ovector[2*n]; ^

github-actions

clang-tidy made some suggestions

github-actions · 2021-04-15T16:36:44Z

src/TTrigger.cpp

            int n = (tabptr[0] << 8) | tabptr[1];
-            auto name = QString::fromUtf8( tabptr + 2, name_entry_size - 3).trimmed();
-            auto capture = QString::fromUtf8(subject + ovector[2*n], ovector[2*n+1] - ovector[2*n]);
+            auto name = QString::fromUtf8(&tabptr[2]).trimmed();


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

auto name = QString::fromUtf8(&tabptr[2]).trimmed(); ^

vadi2 · 2021-04-15T16:46:52Z

We can add a lint ignore line to this code, not much to be done about it.

vadi2 · 2021-04-15T16:47:54Z

Example - //NOLINT(cppcoreguidelines-pro-bounds-pointer-arithmetic)

Delwing · 2021-04-15T20:37:54Z

Do I have to do it for every line or can be done over scope block?

vadi2 · 2021-04-15T20:44:03Z

It doesn't look like scope blocks are supported: https://clang.llvm.org/extra/clang-tidy/

We could turn it off globally, but that doesn't seem such a good idea.

SlySven · 2021-04-15T21:40:39Z

Oh, I just found out what the gsl referred to is - it is the Guidelines-Support Library and https://joshpeterson.github.io/using-span-with-argv gives some examples of using it for C strings...

github-actions · 2021-05-02T09:14:48Z

clang-tidy review says "All clean, LGTM! 👍"

Delwing · 2021-05-02T09:16:28Z

@vadi2 took ma a while, but got finally chance to lean on that :)

vadi2 · 2021-05-02T10:19:17Z

Life happens, thanks for following up on it! I'll look at reviewing it.

Delwing · 2021-06-14T13:38:59Z

@vadi2 if you have any time to look at it please do :)

vadi2 · 2021-06-14T15:50:45Z

Hey I'm away on leave, see if you can get anyone else to review it and feel free to merge when it's good.

vadi2

Looks good! Could you just cover off #5131 (comment) and then merge?

vadi2 · 2021-07-31T09:39:31Z

@Delwing Didn't notice the automerge trigger. Could you still make that small code improvement?

add support for selecting capture groups by their name

c657439

Delwing requested a review from a team as a code owner April 10, 2021 12:08

Delwing requested review from a team April 10, 2021 12:08

Delwing changed the title ~~add support for selecting capture groups by their name~~ selectCaptureGroup will work with named capture groups Apr 10, 2021

github-actions bot reviewed Apr 10, 2021

View reviewed changes

Piotr Wilczynski added 2 commits April 10, 2021 14:17

remove debugs

1b0c9aa

remove redundant space

aa7e198

vadi2 reviewed Apr 10, 2021

View reviewed changes

github-actions bot reviewed Apr 10, 2021

View reviewed changes

add comment for remove of null string

74b98d1

vadi2 reviewed Apr 12, 2021

View reviewed changes

src/TTrigger.cpp Outdated Show resolved Hide resolved

src/TTrigger.cpp Outdated Show resolved Hide resolved

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved

vadi2 assigned Delwing Apr 12, 2021

Piotr Wilczynski added 2 commits April 15, 2021 18:24

fix broken name extraction

c3d46ee

minor adjustment in retrieving group name

a0c5d68

github-actions bot reviewed Apr 15, 2021

View reviewed changes

add NOLINT at named group matching

ae16573

vadi2 assigned vadi2 and unassigned Delwing May 2, 2021

Delwing enabled auto-merge (squash) June 14, 2021 13:38

Delwing merged commit da12bf9 into Mudlet:development Jul 31, 2021

vadi2 approved these changes Jul 31, 2021

View reviewed changes

vadi2 assigned Delwing and unassigned vadi2 Jul 31, 2021

Uh oh!

Conversation

Delwing commented Apr 10, 2021

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Release post highlight

Uh oh!

add-deployment-links bot commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

vadi2 Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

Delwing Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

SlySven Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

Delwing Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 10, 2021

Choose a reason for hiding this comment

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Delwing commented Apr 12, 2021

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

add-deployment-links bot commented Apr 10, 2021 •

edited

Loading