A’ight, cool. I’m with you on this. There is only one thing. I’m not big on the
name BVariantSetVariant(). I left this comment on the PR:
I don't really like the name `BVariantSetVariant`. I get what it is saying but
it sounds funny to me. It also doesn't really say that it isn't setting the
encryption state (I view the encryption state as part of the *variant*). What
about `BVariantSetValue`? That scopes it more to just the value part of the
variant... thoughts?
_______________________________________________________________
FireGiant | Dedicated support for the WiX toolset |
http://www.firegiant.com/
From: Sean Hall [mailto:r.sean.h...@gmail.com]
Sent: Thursday, November 27, 2014 7:59 PM
To: WiX toolset developer mailing list
Subject: Re: [WiX-devs] Just opened bug 4609 against v3.9
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<mailto: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<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<mailto: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<mailto: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=164703151&iu=/4140/ostg.clktrk
_______________________________________________
WiX-devs mailing list
WiX-devs@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wix-devs
