Introduce Bstr scoped guard class to manage BSTR string type #8004
Conversation
dreamer-dead
force-pushed
the
raii-8003-bstr
branch
from
cf4960f
to
d138f59
Compare
dreamer-dead
force-pushed
the
raii-8003-bstr
branch
from
dbd6e5b
to
dea50bd
Compare
dreamer-dead
changed the title
dreamer-dead
changed the title
| static Bstr fromString(std::wstring_view); | ||
|
|
||
| Bstr() = default; | ||
| ~Bstr(); |
There was a problem hiding this comment.
Destructor and other methods that don't throw exceptions could be marked with noexcept keyword
There was a problem hiding this comment.
Will add noexcept specifier for few method.
Also IIRC, the destructor is always has implicit noexcept spec by default
https://en.cppreference.com/w/cpp/language/noexcept_spec
[destructors](https://en.cppreference.com/w/cpp/language/destructor) unless the destructor of any potentially-constructed base or member is potentially-throwing (see below)
There was a problem hiding this comment.
Also IIRC, the destructor is always has implicit
noexceptspec by default
Thanks for sharing this! I was not aware of it. I agree that adding noexcept to other methods would be beneficial.
| return !!bstr_; | ||
| } | ||
|
|
||
| BSTR get() const { |
There was a problem hiding this comment.
A copy to the raw BSTR pointer is returned here (pointer to raw BSTR data), it should return const BSTR instead
There was a problem hiding this comment.
Could you please clarify why const should be added?
const BSTR is just a WCHAR* const type and in my opinion adding const does not affect anything here.
It will be possible to store const BSTR result into BSTR bs variable, see https://godbolt.org/z/v74cvjbxz
There was a problem hiding this comment.
Could you please clarify why const should be added?
I agree that BSTR string can be modified since BSTR is a pointer to a wide-character string, the const qualifier applies to the pointer itself and not the data it points to. The data can still be modified. The const on the return type here is mostly to tell the compiler BSTR that's returned will not be modified (or rather, should not be modified) by the callers. This might not apply here, but when dealing with getters I think it is always good to set the contract upfront and just use const whenever we expect the returned data should not change (return type could be something different in the future and not a pointer)
| } | ||
|
|
||
| BSTR* Bstr::receiveAddress() { | ||
| assert(!bstr_); |
There was a problem hiding this comment.
add a comment on assert usage. Also, I'm not a big fan of adding asserts on production code, I'd prefer to return gracefully if possible.
There was a problem hiding this comment.
Main reason to add this assert is to prevent memory leak and to ensure that the Bstr::bstr_ pointer is null and does not hold any BSTR buffer allocated earlier.
Consider the following scenario:
Bstr bs = Bstr:: fromString(L"foo");
GetFileTime(..., bs.receiveAddress()); // for Debug builds assert can signal here
I personally like the idea of some kind of debug macro helpers with a well defined behaviour like DCHECK and I wasn't sure what to use in this project.
Another approach is to free the buffer every time receiveAddress is called.
There was a problem hiding this comment.
good, thanks for the explanation! You can add this information to a code comment
mike-myers-tob
added
ready for review
This is related to #8003
At the moment I'm trying to add scoped class for
BSTRstring type to support automatic string memory management.Now
Bstrclass has a minimal set of methods/operators that allow to use it in the code.Operators set could be expanded, for example
operator&()can be added or some other convenient helpers.But it can handle the
BSTRmemory and correctly deallocate it.Added few tests and tied to demonstrate how it can be used.
Please, take a look. Looking forward to discuss this PR