Ticket #10051 (closed: fixed)
Make string key based dynamic factories case insensitive
Reported by: | Nick Draper | Owned by: | Nick Draper |
---|---|---|---|
Priority: | major | Milestone: | Release 3.3 |
Component: | Framework | Keywords: | CORE |
Cc: | Blocked By: | ||
Blocking: | Tester: | Michael Wedel |
Description
This is irritating on so many levels.
Add in a CaseInsensitiveDynamicFactory, inheriting from DynamicFactory, that performs key comparisons ignoring case.
Initial design discussed with Martyn.
Attachments
Change History
comment:2 Changed 6 years ago by Nick Draper
- Status changed from assigned to inprogress
re #10051 make default dynamic factories case insensitive
Changeset: cb5c82a84a8c5b58766c880916aa10bfd0fb786d
comment:3 Changed 6 years ago by Nick Draper
re #10051 stricmp is strcasesmp in linux
Changeset: 51fe2d47a95fda9415f0f12b01c51f3e5372355a
comment:4 Changed 6 years ago by Nick Draper
re #10051 need the include <cstring> for linux
Changeset: 3646ec98eebd4ebde6824aafd1b26b9f648aa9d6
comment:5 Changed 6 years ago by Nick Draper
re #10051 need the include <cstring> for linux
Changeset: 7f95d322f955d847248bc0103d495c2bca676cc2
comment:6 Changed 6 years ago by Nick Draper
- Status changed from inprogress to verify
- Resolution set to fixed
email sent to all developers:
Hello, After running into problems with user input being fed straight to a dynamic factory again, it became apparent that the Case sensitivity of our DynamicFactories was not useful, and in fact most of the time is a pain. Let me explain using the UnitFactory for example: If you use: UnitFactory.create(“dSpacing”) // it works But UnitFactory.create(“dspacing”) // it throws an error Clearly it would never make sense to have two units only vary by the case of the unit ID, so therefore it would be better to have our Dynamic Factories be case insensitive when comparing the string keys. Martyn and I had a good think and could not come up with a reason for any of the DynamicFactories to remain case sensitive. In case a reason turns up it is simple to have a particular DynamicFactory Case sensitive, and an example of this is used in the DynamicFactoryTest, you just need to add an optional template argument when inheriting from DynamicFactory. So this give you a case insensitive factory: class MyFactory : public DynamicFactory <MyObjectBaseClasss> And this give you a case sensitive one class MyFactory : public DynamicFactory <<MyObjectBaseClasss, CaseSensitiveStringComparator>
comment:7 Changed 6 years ago by Nick Draper
To Test
- Run CreateSampleWorkspace, set the Unit as "dspacing"
- Plot the resulting workspace, the unit should be correctly set as dSpacing
- Check all unit tests have passed(I've checked this), and all system tests pass.
comment:8 Changed 6 years ago by Michael Wedel
- Status changed from verify to verifying
- Tester set to Michael Wedel
comment:9 Changed 6 years ago by Martyn Gigg
- Status changed from verifying to reopened
- Resolution fixed deleted
It looks like there has been a memory leak created in the DynamicFactoryTest:
comment:10 Changed 6 years ago by Martyn Gigg
On line 70 of DynamicFactoryTest.h, the object return by createUnwrappedInstance is only deleted after the second call so the first one is not deleted.
comment:11 Changed 6 years ago by Michael Wedel
- Status changed from reopened to inprogress
Refs #10051. Fix valgrind warning in DynamicFactoryTest
Changeset: 5621ac212d9161e303e4b3e712f223aebedbeba1
comment:12 Changed 6 years ago by Nick Draper
- Status changed from inprogress to verify
- Resolution set to fixed
comment:14 Changed 6 years ago by Michael Wedel
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/10051_Case_Insensitive_Dynamic_Factories'
Full changeset: 751f476bf48214aee0bd58c8919d0725284af902
comment:15 Changed 6 years ago by Michael Wedel
It works as described in the testing information, as demonstrated by the attached screenshots made before and after the change.
comment:16 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 10893