Repository: ledger/ledger (language: cpp)

Issue:
some code specific to UTF-8 actually accomplishes nothing
There's some code in `commodity_t::parse_symbol(std::istream&, string&)` (src/commodity.cc) for doing something special with non-ASCII UTF-8 characters. However, it doesn't seem to actually make any difference: it copies bytes into a buffer; but all bytes (except "`invalid_chars`") get copied into the buffer anyway. So this special code seems to be pointless.

Is there some subtlety I'm missing? If not, we might as well excise this useless code.


Discussion / hints:
@kcwu since you did some work on UTF-8, maybe you can comment on this ticket.

I guess the code was written that way because the author thought the middle bytes inside UTF-8 may be `invalid_chars`, so he have to parse UTF-8 in order to skip bytes of UTF-8.

However, according to the definition of UTF-8, the intra bytes are always larger than 128, which are not overlapped with `invalid_chars`. Thus, I agree with @kanreki that this function is unnecessary complicated. 



The other idea that occurred to me: it looks as if maybe the intention was to avoid copying a partial multi-byte character in the case where the buffer is just about to fill up. Notice how we keep testing the size of the remaining space in the buffer. And if we don't have enough space, the `bytes` variable doesn't get set. But if that was the intention, then of course we should break out of the loop; instead we just fall through to the last branch of the chain of `if` tests.
(A possibly useful regression test file, depending upon if/how we decide to resolve this.)

[testdata.txt](https://github.com/ledger/ledger/files/8337360/testdata.txt)

@jwiegley can you comment on this since you probably wrote that code?

@kanreki if John doesn't respond (he's busy), can you submit a pull request with some test cases.

My comment from a moment ago might be a little confusing, because that test file might not actually be a sensible regression test if we do decide to remove that code. Rather, I found it useful in *exploring* whether the code was indeed having any effect.

I'll be quite happy to add a test case or two.
I think that UTF-8 code was copy/pasted in from some public domain code I found on the web. No objections to removing needless operations.
Great. I'll do that then. Thanks, John.

Fix patch:
diff --git a/src/commodity.cc b/src/commodity.cc
index 29c04e59d..5ddff5d12 100644
--- a/src/commodity.cc
+++ b/src/commodity.cc
@@ -307,47 +307,14 @@ void commodity_t::parse_symbol(std::istream& in, string& symbol)
       throw_(amount_error, _("Quoted commodity symbol lacks closing quote"));
   } else {
     char * _p = buf;
-    while (_p - buf < 255 && in.good() && ! in.eof() && c != '\n') {
-      std::size_t    bytes = 0;
-      std::ptrdiff_t size  = _p - buf;
-
-      // Check for the start of a UTF-8 multi-byte encoded string
-      if (c >= 192 && c <= 223 && size < 254)
-        bytes = 2;
-      else if (c >= 224 && c <= 239 && size < 253)
-        bytes = 3;
-      else if (c >= 240 && c <= 247 && size < 252)
-        bytes = 4;
-      else if (c >= 248 && c <= 251 && size < 251)
-        bytes = 5;
-      else if (c >= 252 && c <= 253 && size < 250)
-        bytes = 6;
-      else if (c >= 254) // UTF-8 encoding error
-        break;
-
-      if (bytes > 0) {          // we're looking at a UTF-8 encoding
-        for (std::size_t i = 0; i < bytes; i++) {
-          c = in.get();
-          if (in.bad() || in.eof())
-            throw_(amount_error, _("Invalid UTF-8 encoding for commodity name"));
-          *_p++ = c;
-        }
-      }
-      else if (invalid_chars[c]) {
-        break;
-      }
-      else {
+    while (_p - buf < 255 && in.good() && ! in.eof() && ! invalid_chars[c]) {
+      c = in.get();
+      if (c == '\\') {
         c = in.get();
         if (in.eof())
-          break;
-        if (c == '\\') {
-          c = in.get();
-          if (in.eof())
-            throw_(amount_error, _("Backslash at end of commodity name"));
-        }
-        *_p++ = c;
+          throw_(amount_error, _("Backslash at end of commodity name"));
       }
-
+      *_p++ = c;
       c = in.peek();
     }
     *_p = '\0';


Test patch: