Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

nine.fierce.ballads

https://codereview.appspot.com/579310043/diff/549550043/lily/include/source-file.hh
File lily/include/source-file.hh (right):

https://codereview.appspot.com/579310043/diff/549550043/lily/include/source-file.hh#newcode44
lily/include/source-file.hh:44: /* The input data, plus an extra \0 to
terminate */
I haven't looked through this code.  Would changing this from
std::vector<char> to std::string be appropriate?

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

David Kastrup

https://codereview.appspot.com/579310043/diff/549550043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/549550043/lily/source-file.cc#newcode51
lily/source-file.cc:51: characters_.push_back ((char)c);
Frankly, this seems like C++ should offer something better for reading a
whole input file into a buffer (also it seems like the sort of thing
that would be needed on any non-random access file, not just standard
input).

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads
Reviewers: Dan Eble, dak,


https://codereview.appspot.com/579310043/diff/549550043/lily/include/source-file.hh
File lily/include/source-file.hh (right):

https://codereview.appspot.com/579310043/diff/549550043/lily/include/source-file.hh#newcode44
lily/include/source-file.hh:44: /* The input data, plus an extra \0 to
terminate */
On 2020/02/14 12:43:40, Dan Eble wrote:
> I haven't looked through this code.  Would changing this from
std::vector<char>
> to std::string be appropriate?

Done.

https://codereview.appspot.com/579310043/diff/549550043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/549550043/lily/source-file.cc#newcode51
lily/source-file.cc:51: characters_.push_back ((char)c);
On 2020/02/14 13:00:33, dak wrote:
> Frankly, this seems like C++ should offer something better for reading
a whole
> input file into a buffer (also it seems like the sort of thing that
would be
> needed on any non-random access file, not just standard input).

Acknowledged.

Description:
Don't count terminating \0 in Source_file::length

This can confuse the SCM parser, because GUILE can interpret a \0 as
part of an identifer.

Please review this at https://codereview.appspot.com/579310043/

Affected files (+4, -1 lines):
  M lily/include/source-file.hh
  M lily/source-file.cc


Index: lily/include/source-file.hh
diff --git a/lily/include/source-file.hh b/lily/include/source-file.hh
index 4c6275270b4dc8812f855c86c91846be90213b04..3c2e5c3bd87feff7240d7032d5e19825c7b69862 100644
--- a/lily/include/source-file.hh
+++ b/lily/include/source-file.hh
@@ -40,6 +40,8 @@ public:
 private:
   std::vector<char const *> newline_locations_;
   std::istream *istream_;
+
+  /* The input data, plus an extra \0 to terminate */
   std::vector<char> characters_;
   SCM str_port_;
 
Index: lily/source-file.cc
diff --git a/lily/source-file.cc b/lily/source-file.cc
index 205db6971d95d0e03af93e1c4f61b7efa0a9c36e..b08e56922fc8847ac1c5b5c73927623a5883d6c5 100644
--- a/lily/source-file.cc
+++ b/lily/source-file.cc
@@ -49,6 +49,7 @@ Source_file::load_stdin ()
   int c;
   while ((c = fgetc (stdin)) != EOF)
     characters_.push_back ((char)c);
+  characters_.push_back (0);
 }
 
 /*
@@ -357,7 +358,7 @@ Source_file::set_line (char const *pos_str0, ssize_t line)
 size_t
 Source_file::length () const
 {
-  return characters_.size ();
+  return characters_.size () - 1;
 }
 
 char const *



Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

David Kastrup
In reply to this post by nine.fierce.ballads

https://codereview.appspot.com/579310043/diff/561450043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/561450043/lily/source-file.cc#newcode137
lily/source-file.cc:137: data_ = string (&chars[0], chars.size ());
gulp_file_to_string ?

That one apparently also does some kind of logging, so one should check
that using it does not lead to duplications in the log.

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads

https://codereview.appspot.com/579310043/diff/561450043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/561450043/lily/source-file.cc#newcode137
lily/source-file.cc:137: data_ = string (&chars[0], chars.size ());
On 2020/02/14 22:40:51, dak wrote:
> gulp_file_to_string ?
>
> That one apparently also does some kind of logging, so one should
check that
> using it does not lead to duplications in the log.

I prefer keeping the functionality unchanged. We'd print filenames twice
if we used gulp_file_to_string, because Includable_lexer::new_input also
prints [FILENAME .. ]

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

nine.fierce.ballads
In reply to this post by nine.fierce.ballads
LGTM.  Changing to std::string is a definite improvement.  Thanks.


https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc
File lily/engraver.cc (right):

https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc#newcode121
lily/engraver.cc:121: if (!scm_is_pair (props)) {
misplaced braces

https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc#newcode124
lily/engraver.cc:124: };
superfluous semicolon

https://codereview.appspot.com/579310043/diff/557420043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/557420043/lily/source-file.cc#newcode55
lily/source-file.cc:55: return contents of FILENAME. *Not 0-terminated!*
update

https://codereview.appspot.com/579310043/diff/557420043/lily/source-file.cc#newcode84
lily/source-file.cc:84: string dest;
I'd construct with (count, character) rather than construct empty and
resize.

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads

https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc
File lily/engraver.cc (right):

https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc#newcode121
lily/engraver.cc:121: if (!scm_is_pair (props)) {
On 2020/02/15 13:53:16, Dan Eble wrote:
> misplaced braces

Acknowledged.

https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc#newcode124
lily/engraver.cc:124: };
On 2020/02/15 13:53:16, Dan Eble wrote:
> superfluous semicolon

rebase artefact.

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads

https://codereview.appspot.com/579310043/diff/557420043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/557420043/lily/source-file.cc#newcode55
lily/source-file.cc:55: return contents of FILENAME. *Not 0-terminated!*
On 2020/02/15 13:53:17, Dan Eble wrote:
> update

Done.

https://codereview.appspot.com/579310043/diff/557420043/lily/source-file.cc#newcode84
lily/source-file.cc:84: string dest;
On 2020/02/15 13:53:17, Dan Eble wrote:
> I'd construct with (count, character) rather than construct empty and
resize.

Done.

https://codereview.appspot.com/579310043/

Reply | Threaded
Open this post in threaded view
|

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanwenn@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads
commit 73a39333690293208b969d1d8d656f5837467e1c
Author: Han-Wen Nienhuys <[hidden email]>
Date:   Thu Feb 13 09:13:02 2020 +0100

    Store Source_file data in std::string
   


https://codereview.appspot.com/579310043/