brewtarget-devs team mailing list archive

[Philip Lee <rocketman768@xxxxxxxxx>]

Cool, this is looking good.

So you are talking about the instance variable UnitSystem::scaleToUnit. To
take a specific unit system: CelsiusTempUnitSystem::loadMap() does
scaleToUnit.insert(without,Units::celsius). The reason this is failing when
placed in the CelsiusTempUnitSystem constructor is that Units::Celsius is a
static variable, and the only instance of CelsiusTempUnitSystem is also
static (inside UnitSystems::celsiusTempUnitSystem()). There is no guarantee
on order of initialization of static variables, so in your case it seems
that CelsiusTempUnitSystem is initialized before Units::celsius.

http://www.parashift.com/c++-faq-lite/static-init-order.html

One way to fix it would be lazy initialization:

CelsiusTempUnitSystem* UnitSystems::celsiusTempUnitSystem()
{
   static CelsiusTempUnitSystem* c = 0;
   if(!c)
      c = new CelsiusTempUnitSystem();
   return c;
}

On Sat, Oct 11, 2014 at 10:53 AM, mik firestone <mikfire@xxxxxxxxx> wrote:

>
> https://gitorious.org/brewtarget/mikfires-brewtarget/commits/feature/unitrefactor
>
> I don't think a complete and utter rewrite is required. Check that branch
> to see what I am thinking about. It mostly does the right thing -- if you
> enter 5 gal into a field, it will find the appropriate unit based on system
> defaults. If you are using US, you will get 5 us gallons, if you are using
> imperial you will get 5 imperial gallons.
>
> It will do that regardless of what you have selected to use as the display
> unit for the field -- eg, if your calculated batch size is set to
> "Imperial" and your system default is set to "US Customary" and you enter 5
> gal, you will get 5 US gallons. I think I can address that but haven't yet,
> which is why I haven't made the merge request.
>
> It also doesn't handle entering just "5" gracefully, but I think I can fix
> that too.
>
> A few questions. If you dig into the new code, you will find a method
> called loadMap(). What it does is load a QMap with Unit pointers. I would
> really like that to be in the constructor, but when I tried that all of the
> Unit pointers were null. Any idea if I can fix that? I am guessing it is a
> late/early binding kind of problem but ...
>
> Mik
>
>
> On Wed, Oct 8, 2014 at 7:52 PM, Philip Lee <rocketman768@xxxxxxxxx> wrote:
>
>> So, this has been an unreported issue. Welp, file a bug. Maybe it's time
>> to sketch up a replacement for the crappy unit system code now.
>>
>> On Tue, Oct 7, 2014 at 9:06 AM, Mik Firestone <mfirestone@xxxxxxxx>
>> wrote:
>>
>>>  Hmm. Up for a quick thought experiment?
>>>
>>> Assume that I am using SI as my default unit for all things, and have
>>> never played with the units&scales. I read an interesting recipe on line
>>> for a 5 gallon batch and enter it, verbatim, into brewtarget. In the target
>>> batch field, do I get 19L or 22L? Just telling me "gallons" is not enough
>>> to determine if you want US gallons or Imperial gallons. The default
>>> behavior right now is to just assume you meant US gallons. Unless....
>>>
>>> Another experiment, this one not so thoughty. Open any version of
>>> brewtarget after I implemented units&scales. Right click the target batch
>>> size label and set it to US Customary. Enter 5 gal into the field. Right
>>> click the label again, and set the unit to British Imperial. Enter 4.5 gals
>>> (roughly the same size). Right click the label one last time, set it back
>>> to US Customary and enter 5 gal. I am willing to bet the results will be a
>>> bit surprising.
>>>
>>> The nameToUnits hash is keyed by the unit name, e.g., "L" will map to
>>> Units::liters. If you first load the US volume system and then the
>>> Imperial, the Imperial unit system over-writes the entries for the US
>>> volume system. Since each unit is only loaded once, this has some very
>>> interesting side effects. I haven't tried it yet, but I am willing to bet
>>> you can cause the same problem by switching the default from US Customary
>>> -> British Imperial -> US Customary with data entry at each setting. If
>>> that works, it means the "bug" has been around for a while.
>>>
>>> The reason the new code exercises this bug so easily is I am using the
>>> same code path for the right click magic as I am for data input which
>>> causes the hash to be loaded by simply right clicking instead of needing
>>> the data entry. It has taken me weeks to figure that out, by the way.
>>>
>>> Because we have two systems using the same names, I can't really fix the
>>> hash. I simply have no way of understanding what a user means with "5 gal"
>>> -- 19L and 22L are both correct and acceptable. I am going to poke at this
>>> a bit longer to see if I can at least reduce the bug to its original form
>>> but no promises.
>>>
>>> Mik
>>>
>>>  On 10/05/2014 12:22 PM, Philip Lee wrote:
>>>
>>> Bottom line is that it needs to work.
>>>
>>> Can you put this up as a merge request so we can take a look? I won't merge
>>> until your work is done. If I merge before we fix the bug, I'll open a bug
>>> ID to track it and make sure it doesn't go out in a release.
>>>
>>> On Fri, Oct 3, 2014 at 11:34 PM, mik firestone <mikfire@xxxxxxxxx> <mikfire@xxxxxxxxx> wrote:
>>>
>>>
>>>  Ok. I am in the middle of a *huge* refactoring. In the course of simply
>>> (bwahahaha) enabling units and scales for any and all appropriate input
>>> fields, I have pretty much rewritten some of the core functionality in
>>> brewtarget (like Brewtarget::displayAmount()).
>>>
>>> This works, and it works rather nicely and elegantly. But. There is a bug.
>>>
>>> If you switch a volume input field from SI -> us customary -> british
>>> imperial  -> us customary -> SI things will go somewhat pear shaped. This
>>> is a side effect of how the QMap UnitSystem::nameToUnit works, the fact
>>> that us customary and british imperial use the same names for the volume
>>> measures and the way I've changed things around. If any one of those things
>>> changes, the bug goes away.
>>>
>>> This is obviously a bug, but how much of a bug is it? I am not 100%
>>> certain it can be easily fixed. I am 100% certain I do not want to refactor
>>> the UnitSystem stuff right now. If it is considered a big bug, I will
>>> invest the time to determine if I can fix it. If we think it isn't a likely
>>> bug, I will continue with what I'm doing.
>>>
>>> These changes are rather sweeping, so it won't be a simply matter of
>>> backing them out. If this is a big problem and I cannot fix it, I will have
>>> to stop working on this branch.
>>>
>>> Anybody have suggestions?
>>>
>>> mik
>>>
>>> --
>>> In a world of ninja v. pirate, I pilot a Gundam
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
>>> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
>>> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
>>> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
>>> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
>>> _______________________________________________
>>> brewtarget-devs mailing listbrewtarget-devs@lists.sourceforge.nethttps://lists.sourceforge.net/lists/listinfo/brewtarget-devs
>>>
>>>
>>>
>>
>>
>> --
>> Philip G. Lee
>> www.linkedin.com/in/philipgreggorylee
>>
>>
>> ------------------------------------------------------------------------------
>> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
>> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
>> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
>> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
>>
>> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
>> _______________________________________________
>> brewtarget-devs mailing list
>> brewtarget-devs@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.sourceforge.net/lists/listinfo/brewtarget-devs
>>
>>
>
>
> --
> In a world of ninja v. pirate, I pilot a Gundam
>



-- 
Philip G. Lee
www.linkedin.com/in/philipgreggorylee