When I look at the current usage of BVariantCopy, I see two different uses.
The first usage is making a local copy of the variant, this usage matches
the name and what I had in mind when I was modifying it. Here we always
want the local copy's (pTarget) encryption state to match the original
(pSource).
The second usage is being lazy, because what it really wants to do is set
the target's value to the source's value, making BVariantCopy figure out
which type the source is. This is why it makes sense to me to name this
like the other BVariantSet* methods. From there it makes sense to me that
just like the other BVariantSet* methods, the encryption state shouldn't
change.
After typing that out, I now see why you were thinking about
BVariantAssign. VariablesParseFromXml calls BVariantCopy with my second
usage. But it immediately calls BVariantSetEncryption, so it's actually a
third kind of usage - setting the target's value and encryption state at
the same time. On the other hand, I have a feeling that after optimizing
that code it will not be lazy and will call the appropriate BVariantSet*
method directly, with a BVariantSetEncryption call at the end.
This is my long winded answer to Rob's comment on my pull request
<https://github.com/wixtoolset/wix3/pull/181>: the scenario where the
source is encrypted but the target is not encrypted can't happen in the
current code. And I don't see a valid use case for it either. More
comments are always good, though :). I'll add some.
On Tue, Nov 25, 2014 at 6:18 PM, Rob Mensching <r...@firegiant.com> wrote:
> BVariantSetVariant() isn’t bad. Maybe, BVariantAssign()? I think the
> important trick is to control encryption state, right? Almost want a
> tri-state enum:
>
>
>
> HRESULT BVariantAssign(
>
> __in BURN_VARIANT* pTarget,
>
> __in BURN_VARIANT* pSource,
>
> __in BURN_VARIANT_ENCRYPTION state // unencrypted, encrypted, follow
> source
>
> );
>
>
>
> …or something like that.
>
>
>
>
>
> Note2: Now that you mention it, I like it.
>
>
>
> _______________________________________________________________
>
> FireGiant | Dedicated support for the WiX toolset |
> http://www.firegiant.com/
>
>
>
> *From:* Sean Hall [mailto:r.sean.h...@gmail.com]
> *Sent:* Tuesday, November 25, 2014 1:53 PM
> *To:* WiX toolset developer mailing list
> *Subject:* Re: [WiX-devs] Just opened bug 4609 against v3.9
>
>
>
> My original idea was to create a BVariantSetVariant(__in BURN_VARIANT*
> pVariant, __in BURN_VARIANT* pValue), but I scrapped it since it was almost
> an exact copy of BVariantCopy. Maybe that was the right way to go? I just
> wish I was right on the first pull request way back when :)
>
>
>
> Hmm, I wonder if I introduced that inefficiency in the XML parsing?
> Hopefully not.
>
>
>
> Note2: That's why I didn't submit it in a pull request :) I find it's
> much easier to talk about code changes when you can actually see it.
>
>
>
>
>
> On Tue, Nov 25, 2014 at 3:35 PM, Rob Mensching <r...@firegiant.com> wrote:
>
> Ahh, yes, right. The root issue is that the Source encryption state is
> rarely set “correctly”. I 100% agree that using the target’s encryption
> state is weird. I guess what is needed is a “BVariantCopyEx()” (better
> name’s welcome) that is what you had originally to specify the final
> desired encryption state (i.e. you were right).
>
>
>
> I still stand by my comment though that the variables parsing from XML
> using variant as it does is pretty inefficient. <wink/>
>
>
>
>
>
> Note: the FireGiant change was the direct fix that was tested to unblock
> other processes happening here.
>
>
>
>
>
> Note2: The comment “// Encryption here (and decryption later inside
> BVariantCopy) could have been optimized away with breaking change.” is
> going look very strange over time. It’s much better for comments to explain
> why something was done vs. explaining that something that was not done
> could have been done. The latter will be very confusing (or frustrating)
> trying to figure out what the actual issue is. <smile/>
>
>
>
>
>
> _______________________________________________________________
>
> FireGiant | Dedicated support for the WiX toolset |
> http://www.firegiant.com/
>
>
>
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> with Interactivity, Sharing, Native Excel Exports, App Integration & more
> Get technology previously reserved for billion-dollar corporations, FREE
>
> http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
> _______________________________________________
> WiX-devs mailing list
> WiX-devs@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/wix-devs
>
>
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
WiX-devs@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wix-devs
