Yes, I also prefer the first patch. Committed as r147943 with a test. On Wed, Jan 11, 2012 at 2:29 PM, Chandler Carruth <[email protected]> wrote: > Hmm, I see both Anton's point about the ugliness of this code, and your > point about there being no good way to do this. > > The ideal way to manage this in Clang/LLVM land is something along the lines > of the following. > > Define in some class or interface an enum representing the viable modes to > be selected here. Then provide a parse routine which converts a string into > the enum as follows: > > class ... { > enum MyFeatureKind { > MFK_Foo, // "foo" > MFK_Bar // "bar" > }; > > MyFeatureKind parseFeatureString(StringRef S) { > return llvm::StringSwitch<MyFeatureKind>(S) > .Case("foo", MFK_Foo) > .Case("bar", MFK_Bar); > } > } > > > And then in the driver you have your object and write: > > // ... > switch (obj->parseFeatureString(...)) { > case MFK_Foo: > ... > break; > } > > > If there happens to be a one-to-one mapping you can use a table indexed by > the enum, or you can directly use StringSwitch to parse the string and > produce the single value. I think if there are multiple, the switch over the > enum is pretty clear. > > That said, we have a lot of code with if statements in the driver... I'm > actually OK with your original patch's implementation as the existing code > was already structured that way, and it's not clear there is a good place > for this FPU enum to live, get parsed into, etc. If you do see a place, and > would like to refactor things into a structure such as the above, awesome, > but I'd actually prefer you apply your first patch, and *then* do the > refactor. > > However, before you apply the first patch, can you add test cases? You > should be able to add a test case under test/Driver that ensures the proper > flags get forwarded down the chain.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
