https://redmine.named-data.net/https://redmine.named-data.net/favicon.ico?14759811232015-07-02T16:43:34ZNDN project issue tracking systemndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=111672015-07-02T16:43:34ZJunxiao Shi
<ul></ul><p>One of the functions declared in commit:01c44e4c60d33d742192ea65a1198736784ca8c7 is:</p>
<pre><code>std::ostream&
toHex(std::ostream& os, const uint8_t* array, size_t arraySize, bool isUpperCase = true);
</code></pre>
<p>This function can be invoked as:</p>
<pre><code>uint8_t ARR[] { 0x01, 0x02, 0x03 };
toHex(os << "before", ARR, sizeof(ARR), false) << "after";
// -- or, not utilizing the return value --
os << "before";
toHex(os, ARR, sizeof(ARR), false);
os << "after";
</code></pre>
<p>What about allowing this invocation syntax:</p>
<pre><code>uint8_t ARR[] { 0x01, 0x02, 0x03 };
os << "before" << toHex(ARR, sizeof(ARR), false) << "after";
</code></pre>
<p>This could be achieved with a helper class:</p>
<pre><code>class StringHexHelper
{
public:
/** \brief obtain the hexadecimal representation as a string
*/
operator std::string() const;
/** \brief print the hexadecimal representation to a stream
*/
std::ostream&
operator<<(std::ostream& os) const;
private:
StringHexHelper(const uint8_t* buffer, size_t count, bool isUpperCase = true);
friend StringHexHelper toHex(const uint8_t* buffer, size_t count, bool isUpperCase = true);
private:
const uint8_t* m_buffer;
size_t m_count;
bool m_isUpperCase;
};
</code></pre> ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=113202015-07-08T19:40:57ZAlex Afanasyev
<ul></ul><p>I don't think this helper is really necessary, as I cannot think about use cases. Yes, it may simplify writing some of the code, and I would not disagree adding it if somebody really needs it.</p>
<p>The only place (so far) where toHex is really used is inside Digest class (I separated implementation into generic toHex, since I needed to print out hex values of already computed digest in ChronoShare).</p>
<p>The Digest class already provides the same functionality as you are suggesting for StringHexHelper.</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=113212015-07-08T20:01:50ZJunxiao Shi
<ul></ul><p>Reply to note-2:</p>
<p>If you don't want to design as note-1 yet, rename <code>toHex</code> to <code>printHex</code>, and change return type to <code>void</code>.</p>
<ul>
<li>Per rule 2.25, it's wrong to have <code>toHex</code> method name but return a <code>std::ostream</code>.</li>
<li>Returning <code>std::ostream</code> causes ugly code as shown in note-1, unless the return value is ignored.
If the return value is to be ignored, it shouldn't be returned in the first place.</li>
</ul>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=113242015-07-08T21:29:02ZJunxiao Shi
<ul></ul><p>Question from commit:d91269c6a3cbcb62f3510474335b5672244883f9</p>
<pre><code> // @todo AA: Decide what to do with this. I'm not sure this is the correct error model (ignoring)
BOOST_CHECK_EQUAL(unescape(test2), "Invalid escape %ZZ (not a hex value)");
BOOST_CHECK_EQUAL(unescape(test3), "Invalid escape %a (should be two hex symbols)");
BOOST_CHECK_EQUAL(unescape(test4), "Invalid escape %a");
</code></pre>
<p>Answer:</p>
<p>I didn't find a rule in RFC3986 about how to decode an invalid percent-encoded string.<br><br>
I tested <a href="http://php.net/manual/en/function.urldecode.php" class="external">PHP</a>, <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape" class="external">JavaScript</a>, and <a href="https://docs.python.org/2/library/urllib.html#urllib.unquote" class="external">Python</a>.<br>
All of them are returning "%ZZ" and "%a" unchanged, same as the above test case.<br><br>
So I suggest defining this as the intended behavior.</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=114212015-07-10T23:42:19ZJunxiao Shi
<ul></ul><p><code>fromHex</code> function duplicates the functionality of <code>ndn::security::transform::HexDecode</code>.<br>
How should we resolve this duplication?</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=114392015-07-11T19:33:08ZYingdi Yuyuyingdi@gmail.com
<ul></ul><p>I am not aware of this issue until Junxiao pointed out. The toHex/fromHex method in this task is different from ndn::security::transform::HexDecode/HexEncode, because the second one is designed for transformation pipeline. The transform classes may be placed in the middle of a transformation pipeline. Therefore we should keep ndn::security::transform::HexDecode/HexEncode, but I am not sure about toHex/fromHex method...</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=114422015-07-12T06:06:50ZAlex Afanasyev
<ul></ul><p>to/fromHex must exist, as they are convenience functions. Their internal implementation (if somebody really wants to) can be changed to use HexDecode/Encode.</p>
<p>However, HexDecode/Encode does not belong to security namespace.</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=114442015-07-12T09:17:54ZJunxiao Shi
<ul></ul><p>note-5 is not talking about eliminating one of the APIs, but is talking about using one to implement the other.</p>
<p>In commit:7e779655a350d4b0292387f4f11025e5978ec168, <code>fromHex</code> is implemented with using CryptoPP.<br><br>
This implementation must be replaced with <code>ndn::security::transform::HexDecode</code> to achieve the <a class="issue tracker-3 status-5 priority-2 priority-default closed parent" title="Task: Adding libcrypto-based crypto support (Closed)" href="https://redmine.named-data.net/issues/2949">#2949</a> goal of eliminating CryptoPP.</p>
<p><code>printHex</code> is implemented with <code><iostream></code>.<br><br>
Not changing to <code>ndn::security::transform::HexEncode</code> is acceptable to me, because the change would incur 2 additional copies in:<br>
(1) constructing a <code>std::stringstream</code> for <code>StreamSource</code>, and<br>
(2) <code>HexEncode</code> output buffer before writing to <code>StreamSink</code>.</p>
<p>Now the question is how to resolve the conflict of <code>fromHex</code>'s usage of CryptoPP:</p>
<ul>
<li>Proceed with CryptoPP implementation, and change to <code>HexDecode</code> under a subtask of <a class="issue tracker-3 status-5 priority-2 priority-default closed parent" title="Task: Adding libcrypto-based crypto support (Closed)" href="https://redmine.named-data.net/issues/2949">#2949</a>.</li>
<li><a class="issue tracker-2 status-5 priority-2 priority-default closed child" title="Feature: Crypto transformation concatenation (Closed)" href="https://redmine.named-data.net/issues/3009">#3009</a> blocks <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: Improvements of string helpers (Closed)" href="https://redmine.named-data.net/issues/3006">#3006</a>.</li>
</ul>
<p>I have no preference between these two options. Assignee needs to make a choice.</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=114482015-07-12T13:51:40ZYingdi Yuyuyingdi@gmail.com
<ul></ul><p>Junxiao Shi wrote:</p>
<blockquote>
<p>Now the question is how to resolve the conflict of <code>fromHex</code>'s usage of CryptoPP:</p>
<ul>
<li>Proceed with CryptoPP implementation, and change to <code>HexDecode</code> under a subtask of <a class="issue tracker-3 status-5 priority-2 priority-default closed parent" title="Task: Adding libcrypto-based crypto support (Closed)" href="https://redmine.named-data.net/issues/2949">#2949</a>.</li>
<li><a class="issue tracker-2 status-5 priority-2 priority-default closed child" title="Feature: Crypto transformation concatenation (Closed)" href="https://redmine.named-data.net/issues/3009">#3009</a> blocks <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: Improvements of string helpers (Closed)" href="https://redmine.named-data.net/issues/3006">#3006</a>.</li>
</ul>
</blockquote>
<p>I prefer the first one. Given <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: Improvements of string helpers (Closed)" href="https://redmine.named-data.net/issues/3006">#3006</a> is in master branch, and <a class="issue tracker-2 status-5 priority-2 priority-default closed child" title="Feature: Crypto transformation concatenation (Closed)" href="https://redmine.named-data.net/issues/3009">#3009</a> will not be merged into master branch in a near future (e.g., next two weeks).</p>
ndn-cxx - Feature #3006: Improvements of string helpershttps://redmine.named-data.net/issues/3006?journal_id=117612015-07-26T23:17:38ZJunxiao Shi
<ul><li><strong>Status</strong> changed from <i>Code review</i> to <i>Closed</i></li></ul>