szaszm commented on a change in pull request #1211:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1211#discussion_r749244836



##########
File path: cmake/Extensions.cmake
##########
@@ -22,10 +22,31 @@ define_property(GLOBAL PROPERTY EXTENSION-OPTIONS
 
 set_property(GLOBAL PROPERTY EXTENSION-OPTIONS "")
 
+set(extension-build-info-file 
"${CMAKE_CURRENT_BINARY_DIR}/ExtensionBuildInfo.cpp")
+file(GENERATE OUTPUT ${extension-build-info-file}
+    CONTENT "\
+    #include \"utils/Export.h\"\n\
+    #ifdef BUILD_ID_VARIABLE_NAME\n\
+    EXTENSIONAPI extern const char* const BUILD_ID_VARIABLE_NAME = 
\"__EXTENSION_BUILD_IDENTIFIER_BEGIN__${BUILD_IDENTIFIER}__EXTENSION_BUILD_IDENTIFIER_END__\";\n\

Review comment:
       Normally `extern` is used do declare a global variable in a header that 
is then defined in some implementation file without violation ODR. Otherwise a 
global variable declaration would also be a definition. I don't understand why 
having an initializer here is even legal c++, since it should go to the 
separate definition, not the declaration.
   
   According to [this](https://stackoverflow.com/a/57900212) stackoverflow 
answer, having `extern` here is the same as not having it, because the 
initializer makes a definition and all globals have external linkage by default.
   
   What do you think about having a global function instead that returns a 
`const char*` to the magic string? I think that shouldn't be stripped from the 
binary even with LTO.




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to