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

[Viktor Kutuzov]

Please find the patch attached.

Is it Ok to submit?

However, the SubtargetFeature constructor accepts a string as an argument and parse it as a comma-separated list. There is also the 
SubtargetFeature::setString method which does the same.

While I'm changing the SubtargetFeature, would you like me to change the SubtargetFeature constructor to accept array of StringRef's 
and remove setString method?

-Viktor

----- Original Message ----- 
From: "Chris Lattner" <clattner at apple.com>
To: "Daniel Dunbar" <daniel at zuster.org>
Cc: "Viktor Kutuzov" <vkutuzov at accesssoftek.com>; <llvm-commits at cs.uiuc.edu>
Sent: Monday, November 23, 2009 9:37 AM
Subject: Re: [llvm-commits] [llvm] r89516 - in /llvm/trunk: include/llvm/Target/SubtargetFeature.h lib/Target/SubtargetFeature.cpp 
tools/lto/LTOCodeGenerator.cpp



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
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-lto-codegen-subtargetfeature-remove_ext_functionality.diff
Type: application/octet-stream
Size: 2172 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20091123/7edba59e/attachment.obj>