[PATCH] D37778: [lit] Add a shared llvm configuration library, and use it in llvm

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:6
+
+def _is_true_like(lit_config, value):
+    if value is None:
----------------
dlj wrote:
> It might make sense to move this into the LLVMConfig class, so you don't have to keep passing lit_config.
> 
> It also looks like this is always called with attributes on config, so you could roll in the getattr()s here as well.
Hmm, even better might be perhaps to move this into `lit.util`.  There's nothing llvm specific about this


================
Comment at: llvm/utils/lit/lit/llvm/config.py:118
+            current_value = self.config.environment[variable]
+            items = [norm(x) for x in current_value.split(os.path.pathsep)]
+            try:
----------------
dlj wrote:
> dlj wrote:
> > Are you sure it's safe normalize the existing values?
> > 
> > It seems like it should be OK, since you won't be re-normalizing all the time. Doing normalized comparisons would be a bit more complex, too.
> I think you want this to be:
> 
> os.path.pathsep.split(current_value)
Yea, the problem is that I really need to compare whether or not the paths are "equivalent".  This means handling case-sensitivity and normalizing slashes, otherwise the string compare won't work.  If someone puts `D:/src/llvm` into a variable, and then I add `D:\src\llvm`, they should match.  Luckily this code isn't really in a hot path, so performance doesn't matter, and I can't think of a reason why it would be unsafe.  On non-Windows platforms, `norm` is a no-op

> I think you want this to be:  os.path.pathsep.split(current_value)

Is there a difference?


================
Comment at: llvm/utils/lit/lit/llvm/config.py:146
+
+        if re.search(r'ON', llvm_config_cmd.stdout.read().decode('ascii')):
+            self.config.available_features.add(feature)
----------------
dlj wrote:
> I think you might want to use this pattern instead:
> 
>   output, _ = llvm_config_cmd.communicate()
>   if re.search('ON', output.decode('ascii')):
>     # ...
> 
> That will handle the blocking semantics correctly on the subprocess's file handles. It also eliminates the need for the wait() below.
Good suggestion.  It's almost as if centralizing all this copied code into a single place has tangible value :D


https://reviews.llvm.org/D37778