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

[Daniel Dunbar]

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.

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
>