The FromData method causes a cast alignment warning when building with clang and it's a weird kind of method anyways. It would be good to use a more type safe approach.
Example warning from Clang: nsSMILCSSProperty.cpp In file included from /Users/ehsanakhgari/moz/mozilla-central/content/smil/nsSMILCSSProperty.cpp:40: In file included from /Users/ehsanakhgari/moz/mozilla-central/content/smil/nsSMILCSSProperty.h:44: In file included from ../../dist/include/nsIAtom.h:19: ../../dist/include/nsStringBuffer.h:109:18: warning: cast from 'char *' to 'nsStringBuffer *' increases required alignment from 1 to 4 [-Wcast-align] return (nsStringBuffer*) ( ((char*) data) - sizeof(nsStringBuffer) ); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
6 years ago
Created attachment 531983 [details] [diff] [review] Patch (v1)
This adds extra arithmetic on all the atom string access, right? That seems undesirable.... On the other hand, I guess before we had that extra arithmetic at the FromData() callsites.
I don't understand why this only touches atom code? Surely our string code uses FromData in order to addref/release string buffers?
Another solution to this is to not cast down to char* and do the adjustment there, but rather to cast to PRUint32* and adjust the offset.
(In reply to comment #4) > This adds extra arithmetic on all the atom string access, right? That seems > undesirable.... > > On the other hand, I guess before we had that extra arithmetic at the > FromData() callsites. In the worst case we could always have a type safe wrapper of PRUnichar* that knows it's actually nsStringBuffer and but keeps the arithmetic in the same places.
That would work for atoms, but not for strings.
Can somebody try to explain to me why nsStringBuffer is necessary at all? I have a hard time figuring out what problem it's trying to solve, and I wonder if there isn't a better way to solve that problem.
A nsStringBuffer represents a refcounted buffer which holds characters. This allows us to share, rather than copy, data between nsString instances. (IIRC there was a very large drop both in used cycles and in used memory when we added them, though the cycle drop was hard to measure since the same patch made other perf-improving changes as well). When modifying a string which currently uses a nsStringBuffer we do a copy if the buffers refcount is larger than 1. Otherwise we modify the buffer in-place. I.e. nsStringBuffers is how we implement copy-on-write for strings. The reason atoms use nsStringBuffers is because atoms are basically interned strings. So we can share data between strings and atoms, both when creating atoms, and when reading string-data out from atoms. We also use nsStringBuffers to allow nsAttrValues to share data with strings.
We also use nsStringBuffers to implement the limited sharing we have between XPCOM strings and JSString. Also, some places in style data hold string buffers directly. Templating stringbuffers on the char type would be a fine idea if it helps things.
Comment on attachment 531983 [details] [diff] [review] Patch (v1) I really don't think that this is a useful patch. We should template nsStringBuffer, in an ideal world, but the pointer math it does isn't actually wrong, and making this change just to reduce the incidence of warnings doesn't seem helpful.
Created attachment 537861 [details] [diff] [review] Patch (v2) How about just silencing the warnings, like we did in bug 659546? These warnings are extremely annoying when building with Clang, and hinder the usage of the rest of the compiler diagnostics (which are actually useful!).
Created attachment 537862 [details] [diff] [review] Patch (v2) Sorry, the previous version contained unrelated debugging changes.
Comment on attachment 537862 [details] [diff] [review] Patch (v2) Might be nice to use C++-style reinterpret_cast, but sure.
Created attachment 538131 [details] [diff] [review] Let the compiler do the pointer arithmetics Actually Neil gave me a better idea on IRC. How about we let the compiler do its job (figuring out the pointer arithmetics) instead of doing that manually?
Comment on attachment 538131 [details] [diff] [review] Let the compiler do the pointer arithmetics Now THIS I can get behind! :)