#990:Implemented escape sequence for jar option with trailing whitespaces #472

tomuben · 2024-10-25T19:52:43Z

related to exasol/script-languages-release#990

tkilias · 2024-10-28T09:40:27Z

exaudfclient/base/javacontainer/script_options/string_ops.cc

+
+void replaceTrailingEscapeWhitespaces(std::string & s) {
+    if (s.size() > 0) {
+        const size_t lastIdx = s.find_last_not_of(" \t\v\f");


tkilias · 2024-10-28T09:42:40Z

exaudfclient/base/javacontainer/script_options/string_ops.cc

+    if (s.size() > 0) {
+        const size_t lastIdx = s.find_last_not_of(" \t\v\f");
+        if (lastIdx != std::string::npos) {
+            size_t rtrimIdx = lastIdx + 1;


first whitespace after last not whitespace

tkilias · 2024-10-28T09:50:17Z

exaudfclient/base/javacontainer/script_options/string_ops.cc

+        if (nBackslashes > 0) {
+            s = s.erase(pos-nBackslashes, (nBackslashes>>1) );
+        }
+        rtrimIdx = pos + 1 - (nBackslashes>>1);


extract nBackslashes>>1 into variable with name halfBackslasches or so

tkilias · 2024-10-28T09:52:08Z

exaudfclient/base/javacontainer/script_options/string_ops.cc

+    else {
+        //<replacement> does belong to an escape sequence
+        //Delete half of the backslashes because they belong to the escape sequences + 1 of the <replacement>
+        s = s.erase(pos-nBackslashes, (nBackslashes>>1)+1 );


same as https://github.com/exasol/script-languages/pull/472/files#r1818735696

tkilias · 2024-10-28T09:57:13Z

exaudfclient/base/javacontainer/script_options/string_ops.h

@@ -21,7 +21,7 @@ inline void ltrim(std::string &s) {
 }

 inline void rtrim(std::string &s) {
-    s.erase(std::find_if(s.rbegin(), s.rend(), [](unsigned char ch) {
+    s.erase(std::find_if(s.rbegin(), s.rend(), [&](unsigned char ch) {


do we need to capture

…aces

Nicoretti · 2024-10-28T15:49:14Z

exaudfclient/base/javacontainer/script_options/converter.cc

    if (value != "") {
-        m_jvmOptions.push_back("-Dexasol.scriptclass=" + trimmedValue);
+        m_jvmOptions.push_back("-Dexasol.scriptclass=" + value);
    }


if value.empty() { return } m_jvmOptions.push_back("-Dexasol.scriptclass=" + value);

Generally, it is easier to read if you add preconditions with an early return at the top and include the default logic in the default flow.

Ah, I knew I another point

Nicoretti · 2024-10-28T15:58:30Z

exaudfclient/base/javacontainer/script_options/string_ops.cc

+    uint32_t retVal(0);
+    while (pos >= 0 && s[pos--] == '\\') retVal++;
+    return retVal;


I think something along the lines below should do too.

Suggested change

uint32_t retVal(0);

while (pos >= 0 && s[pos--] == '\\') retVal++;

return retVal;

// including <algorithm> is required

return std::count (s.cbegin(), s.cend(), '\\');

Hm, but does this work, because we need to stop, when we encounter now backslash

Nicoretti · 2024-10-28T16:04:26Z

exaudfclient/base/javacontainer/script_options/string_ops.cc

+inline size_t replaceOnlyBackslashSequencesButKeepChar(std::string & s, size_t backslashStartIdx, size_t nBackslashes) {
+    const uint32_t nHalfBackslashes = (nBackslashes>>1);
+    if (nHalfBackslashes > 0) {
+        s = s.erase(backslashStartIdx, nHalfBackslashes );
+    }
+    const size_t newBackslashEndIdx = backslashStartIdx + nHalfBackslashes;
+    return newBackslashEndIdx + 1; //+1 because of the last None-Whitespace character we need to keep
+}
+
+inline size_t replaceBackslashSequencesAndWhitespaceSequence(std::string & s, size_t backslashStartIdx,
+                                                                size_t nBackslashes, const char* replacement) {
+    const uint32_t nHalfBackslashes = (nBackslashes>>1);
+    s = s.erase(backslashStartIdx, nHalfBackslashes+1 ); //Delete also backslash of whitespace escape sequence
+    const size_t newBackslashEndIdx = backslashStartIdx + nHalfBackslashes;
+    s = s.replace(newBackslashEndIdx, 1, replacement);
+    return newBackslashEndIdx + 1; //+1 because of the replaced whitespace character
+}
+
+inline size_t replaceCharAtPositionAndBackslashes(std::string & s, size_t pos, const char* replacement) {
+    const uint32_t nBackslashes = countBackslashesBackwards(s, pos-1);
+
+    const size_t backslashStartIdx = pos-nBackslashes;
+    if(nBackslashes % 2 == 0) {
+        return replaceOnlyBackslashSequencesButKeepChar(s, backslashStartIdx, nBackslashes);
+    }
+    else {
+        return replaceBackslashSequencesAndWhitespaceSequence(s, backslashStartIdx, nBackslashes, replacement);
+    }
+}
+
+void replaceTrailingEscapeWhitespaces(std::string & s) {
+    if (s.size() > 0) {
+        const size_t lastNoneWhitespaceIdx = s.find_last_not_of(" \t\v\f");
+        if (lastNoneWhitespaceIdx != std::string::npos) {
+            size_t firstWhitespaceAfterNoneWhitespaceIdx = lastNoneWhitespaceIdx + 1;
+            if (s.size() > 1) {
+                if(s[lastNoneWhitespaceIdx] == 't') {
+                    firstWhitespaceAfterNoneWhitespaceIdx = replaceCharAtPositionAndBackslashes(s, lastNoneWhitespaceIdx, "\t");
+                } else if (s[lastNoneWhitespaceIdx] == '\\' && s[lastNoneWhitespaceIdx+1] == ' ') {
+                    firstWhitespaceAfterNoneWhitespaceIdx = replaceCharAtPositionAndBackslashes(s, lastNoneWhitespaceIdx+1, " ");
+                } else if (s[lastNoneWhitespaceIdx] == 'f') {
+                    firstWhitespaceAfterNoneWhitespaceIdx = replaceCharAtPositionAndBackslashes(s, lastNoneWhitespaceIdx, "\f");
+                } else if (s[lastNoneWhitespaceIdx] == 'v') {
+                    firstWhitespaceAfterNoneWhitespaceIdx = replaceCharAtPositionAndBackslashes(s, lastNoneWhitespaceIdx, "\v");
+                }
+            }
+            if (firstWhitespaceAfterNoneWhitespaceIdx != std::string::npos &&
+                firstWhitespaceAfterNoneWhitespaceIdx < s.size()) {
+                s = s.substr(0, firstWhitespaceAfterNoneWhitespaceIdx); //Right Trim the string
+            }
+        } else {
+            s = "";
+        }
+    }
+}
+
+} //namespace StringOps
+
+
+} //namespace JavaScriptOptions
+
+} //namespace SWIGVMContainers


consider using std::transform, std::replace, std::replace_if, ... for replacing substrings.
https://en.cppreference.com/w/cpp/algorithm/transform

I think, this doesn't apply here, because we replace either a single position or we replace a double backslasches with a single backslasches, by removing half of the backslasches

tkilias reviewed Oct 28, 2024

View reviewed changes

tomuben added 2 commits October 28, 2024 11:37

#990:Implemented escape sequence for jar option with trailing whitesp…

56202bf

…aces

Refactored implementation

506db77

tomuben force-pushed the refactoring/990_implement_escape_sequence_for_jar_option_with_trailing_whitespaces branch from 0020462 to 506db77 Compare October 28, 2024 14:37

Nicoretti reviewed Oct 28, 2024

View reviewed changes

Findings from review

87a667f

tkilias approved these changes Oct 28, 2024

View reviewed changes

tomuben merged commit cf43d08 into master Oct 29, 2024
24 checks passed

tomuben deleted the refactoring/990_implement_escape_sequence_for_jar_option_with_trailing_whitespaces branch October 29, 2024 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#990:Implemented escape sequence for jar option with trailing whitespaces #472

#990:Implemented escape sequence for jar option with trailing whitespaces #472

tomuben commented Oct 25, 2024

tkilias Oct 28, 2024

tkilias Oct 28, 2024

tkilias Oct 28, 2024

tkilias Oct 28, 2024

tkilias Oct 28, 2024

Nicoretti Oct 28, 2024

Nicoretti Oct 28, 2024

tkilias Oct 28, 2024

Nicoretti Oct 28, 2024

tkilias Oct 28, 2024

tkilias Oct 28, 2024

Nicoretti Oct 28, 2024

tkilias Oct 28, 2024

#990:Implemented escape sequence for jar option with trailing whitespaces #472

#990:Implemented escape sequence for jar option with trailing whitespaces #472

Conversation

tomuben commented Oct 25, 2024

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