[GitHub] [pulsar] BewareMyPower commented on issue #11632: [C++] pulsar-client-cpp stability enhancements and code cleanup

[GitBox <gi...@apache.org>]

BewareMyPower commented on issue #11632:
URL: https://github.com/apache/pulsar/issues/11632#issuecomment-897470434


   LGTM, here're my comments.
   
   > - Enable many more warnings (-Wall, -Wvla, -Wformat-security, etc)
   > - Enforce warnings as errors to prevent changes being submitted with warnings
   > - Fix all the subsequent warnings that will be generated
   
   Great. Currently there're a lot of compile warnings and it would be better to eliminate all these warnings. 
   
   > Fix compatibility with boost 1.41 (the documentation claims it should be compatible, but it's not, and that happens to be the version we're stuck on).
   
   It would be helpful for Boost incompatibilities. But I just have one concern. Since some methods of newer Boost might not be available for older Boost, it's easy to break the Boost incompatibility even after you fixed the incompatibility. But I don't think it's good to downgrade the Boost dependency of image for CI.
   
   > Use consistent format for #includes of local files (some have a /lib prefix, others don't)
   
   I've just done this work several days ago but forgot to submit a PR. I can open a PR later. The original purpose is that currently Pulsar C++ client doesn't obey PImpl idiom well and some headers have many included headers, which adds a lot of unnecessary compile time if some files are changed.
   
   > Run the tests via ASAN or Valgrind, if available (not sure how easy this will be yet...)
   
   Looks good. But I'm also not sure how easy this will be :(
   
   > Update syntax to take advantage of more C++11 features (like std::make_shared, move semantics, etc)
   
   Great. There're much old legacy code currently, like explicit iterator in an iteration, a non-atomic variable with a mutex for thread safety, etc.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org