[llvm-commits] [llvm] r89516 - in /llvm/trunk: include/llvm/Target/SubtargetFeature.h lib/Target/SubtargetFeature.cpp tools/lto/LTOCodeGenerator.cpp

[Chris Lattner]

On Nov 22, 2009, at 9:41 AM, Daniel Dunbar wrote:

> On Sun, Nov 22, 2009 at 6:03 AM, Chris Lattner <clattner at apple.com> wrote:
>> 
>> On Nov 20, 2009, at 4:00 PM, Viktor Kutuzov wrote:
>> 
>>> Author: vkutuzov
>>> Date: Fri Nov 20 18:00:02 2009
>>> New Revision: 89516
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=89516&view=rev
>>> Log:
>>> Added two SubtargetFeatures::AddFeatures methods, which accept a comma-separated string or already parsed command line parameters as input, and some code re-factoring to use these new methods.
> 
> I don't think this code belongs in SubtargetFeatures at all. All it is
> doing is calling AddFeature on each string, the client is perfectly
> capable of doing this, which obviates thinking about how best to pass
> the vector.

Yes, I agree.  Viktor, please remove this part of the patch, pushing the logic into the LTO client.

-Chris

> 
> Similarly, AddFeatures shouldn't impose some kind of discipline like
> comma separate strings, clients should handle this if it is what they
> have (and StringRef::split makes it easy for them to split the
> string).
> 
> - Daniel
> 
>> Ok, a couple comments below:
>> 
>>> +++ llvm/trunk/include/llvm/Target/SubtargetFeature.h Fri Nov 20 18:00:02 2009
>>> @@ -22,6 +22,7 @@
>>> #include <vector>
>>> #include <cstring>
>>> #include "llvm/ADT/Triple.h"
>>> +#include "llvm/Support/CommandLine.h"
>>> #include "llvm/System/DataTypes.h"
>> 
>> Please drop this #include.
>> 
>>> @@ -93,6 +94,12 @@
>>>   /// Adding Features.
>>>   void AddFeature(const std::string &String, bool IsEnabled = true);
>>> 
>>> +  /// Add a set of features from the comma-separated string.
>>> +  void AddFeatures(const std::string &String);
>> 
>> This should take a StringRef instead of std::string.
>> 
>>> +
>>> +  /// Add a set of features from the parsed command line parameters.
>>> +  void AddFeatures(const cl::list<std::string> &List);
>> 
>> cl::list inherits from std::vector, so you should be able to pass in a std::vector<std::string> directly.  However, it would be much much better to expose this as taking an array of StringRef's and require the caller to do the unpacking.
>> 
>> -Chris
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>