KalleOlaviNiemitalo commented on code in PR #3266:
URL: https://github.com/apache/avro/pull/3266#discussion_r1931743645
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -16,26 +16,63 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-#include "CustomAttributes.hh"
-#include "Exception.hh"
+
+#if defined(__clang__)
+// Even though CustomAttributes::ValueMode::STRING is deprecated, we still
have to
+// handle/implement it.
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#endif
+
#include <map>
#include <memory>
+#include "CustomAttributes.hh"
+#include "Exception.hh"
+
+#include "json/JsonDom.hh"
+
namespace avro {
+CustomAttributes::CustomAttributes(ValueMode valueMode) {
+ switch (valueMode) {
+ case ValueMode::STRING:
+ case ValueMode::JSON:
+ valueMode_ = valueMode;
+ break;
+ default:
+ throw Exception("invalid ValueMode: " +
std::to_string(static_cast<int>(valueMode)));
+ }
+}
+
std::optional<std::string> CustomAttributes::getAttribute(const std::string
&name) const {
- std::optional<std::string> result;
- std::map<std::string, std::string>::const_iterator iter =
- attributes_.find(name);
+ auto iter = attributes_.find(name);
if (iter == attributes_.end()) {
- return result;
+ return {};
}
- result = iter->second;
- return result;
+ return iter->second;
}
void CustomAttributes::addAttribute(const std::string &name,
const std::string &value) {
+ // Validate the incoming value.
+ //
+ // NOTE: This is a bit annoying that we accept the data as a string
instead of
+ // as an Entity. That means the compiler must convert the value to a
string only
+ // for this method to convert it back. But we can't directly refer to the
+ // json::Entity type in the signatures for this class (and thus cannot
accept
+ // that type directly as a parameter) because then it would need to be
included
+ // from a header file: CustomAttributes.hh. But the json header files are
not
+ // part of the Avro distribution (intentionally), so CustomAttributes.hh
cannot
+ // #include any of the json header files.
+ const std::string &jsonVal = (valueMode_ == ValueMode::STRING)
+ ? std::move("\"" + value + "\"")
+ : value;
Review Comment:
I worried that the temporary string `"\"" + value + "\""` might be destroyed
too early, and `const std::string &jsonVal` would then be a dangling reference,
like in
<https://stackoverflow.com/questions/75372382/stdmove-and-lifetime-of-temporary-objects>.
But from looking at disassembly in [Compiler
Explorer](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:2,endLineNumber:13,positionColumn:2,positionLineNumber:13,selectionStartColumn:2,selectionStartLineNumber:13,startColumn:2,startLineNumber:13),source:'%23include+%3Cstring%3E%0A%0Aint+dummy()%3B%0A%0Aenum+class+ValueMode+%7B+STRING+%3D+919+%7D%3B%0A%0Aint+square(ValueMode+valueMode_,+const+std::string+%26value)+throw()+%7B%0A++++const+std::string%26+jsonVal+%3D+(valueMode_+%3D%3D+ValueMode::STRING)%0A++++++++%3F+std::move(%22%5C%22%22+%2B+value+%2B+%22%5C%22%22)%0A++++++++:+value%3B%0A++++dummy()%3B%0A++++return+jsonVal.size()%3B%0A%7D'),l:'5',n:'0
',o:'C%2B%2B+source+%231',t:'0')),k:23.073416698386744,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:compiler,i:(compiler:clang_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'',overrides:!((name:stdver,value:c%2B%2B23)),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(trunk)+(Editor+%231)',t:'0')),k:76.92658330161326,l:'4',n:'0',o:'',s:0,t:'0')),l:'2',n:'0',o:'',t:'0')),version:4),
it seems to be actually OK:
* If `valueMode_ == ValueMode::STRING`, then the first temporary is copied
(by clang) or moved (by gcc) to another temporary std::string, the reference
`const std::string &jsonVal` is bound to the second temporary, and the lifetime
of the second temporary is extended to match the reference.
* If `valueMode_ != ValueMode::STRING`, then the parameter `const
std::string &value` is copied to a temporary std::string, the reference `const
std::string &jsonVal` is bound to the temporary, and the lifetime of the
temporary is extended to match the reference.
OTOH, the machine code would be a bit shorter without std::move, perhaps
more efficient too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@avro.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
