search

saleae / SampleAnalyzer

Sample Analyzer for the Analyzer SDK
MIT License
151 stars 183 forks source link

`std::auto_ptr<...> needs to have dll-interface` warning on Visual Studio 2022 (MSBuild 17) #32

Open starthal opened 3 months ago

starthal commented 3 months ago

I'm not sure if this is related to #18.

We see the warning std::auto_ptr<...> needs to have dll-interface when building with VS2022 (MSBuild 17.x).

This appears to be ignored in one of the official analyzers: https://github.com/saleae/modbus-analyzer/blob/0b2845dc1d2bb1322aae695a32b94d42d9bb2012/src/ModbusAnalyzer.h#L28-L30

Is the warning relevant? If not, is it appropriate to suppress the warning in the header as it was done there?

Marcus10110 commented 2 months ago

Sorry about this!

  1. We don't expose any of the auto_ptrs outside of the class, just raw pointers to those items, so this isn't a problem in practice. However as a strong proponent of warnings as errors (Which we have enabled in our main codebases), I realize this can be really annoying.
  2. The correct solution is to completely remove std::auto_ptr, something that we put in there back in the old days when we were still a 2 person company, before we had even fully adopted C++11! It was actually the wrong tool for the job then, and even more so now. All of the auto_ptr members of the SimpleSerialAnalyzerSettings can actually just be data members, not pointers. In SimpleSerialAnalyzer.h, mSettings could also just be normal data member, no need for a pointer, and mResults could probably be a regular data member, as long as it supports being cleared out & reset each time SetupResults() is called. In all cases, conversion to std::unique_ptr would work and would probably require zero additional code changes too, without thinking about it.
Marcus10110 commented 2 months ago

Ok, I've just removed std::auto_ptr. A bit of a side quest, but that was bothering me more. https://github.com/saleae/SampleAnalyzer/pull/33

However that does not scrub this warning from the codebase, and since the fix involves one instance of std::unique_ptr, there is now a new instance of the warning for that.

There are 2 ways to fix the warning.

  1. Suppress or ignore the warning. (this could be done in code, like the example you linked, or in the CMakeLists.txt file)
  2. Change the code to avoid the issue.

To change this in code, we would need to add the ANALYZER_EXPORT directive to the remaining classes, and we would need to move the stl data members of each class into an implementation class, and hide that from the header, as shown here: https://stackoverflow.com/a/8271841/403852

I don't think the extra memory management work (specifically, manually deleting the implementation details on destruction) would be worth eliminating this warning, I think option 1 is the best path forward.

I have a follow up PR that does this here: https://github.com/saleae/SampleAnalyzer/pull/34

To be honest though, I'm not thrilled with littering the headers with so many pragmas. I'll re-consider the CMake option later.

What do you think?

starthal commented 2 months ago

FWIW, pimpl can be done with unique_ptr, as in the example here.