[PATCH] D34855: [lit] Factor out listdir logic shared by different test formats.

David L. Jones via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 29 18:52:11 PDT 2017


dlj added inline comments.


================
Comment at: utils/lit/lit/util.py:139
+            filename in exclude_filenames or
+            not any(filename.endswith(sfx) for sfx in suffixes)):
+            continue
----------------
modocache wrote:
> Note that this is a subtle change in behavior from `os.path.splitext()`. Consider a file without an extension, such as `TARGETS`:
> 
> ```
> >>> os.path.splitext('TARGETS')
> ('TARGETS', '')
> ```
> 
> The previous logic, which used `os.path.splitext()`, would not include the file, even if `'TARGETS'` were included as a suffix. This new logic would include it, since `TARGETS.endswith('TARGETS')` returns `True`.
> 
> I'm not sure if any consumers of lit rely on this behavior... but if you didn't intend to make a change, perhaps keep the old behavior for now?
Yup, you are correct... I think this is OK as-is, though, since splitext includes the trailing dot:
 
In [2]: os.path.splitext('TARGETS')
Out[2]: ('TARGETS', '')

In [3]: os.path.splitext('x.TARGETS')
Out[3]: ('x', '.TARGETS')

Any existing suffixes in lit configs would need to have a dot in order to match. For example:

https://reviews.llvm.org/diffusion/L/browse/lld/trunk/test/lit.cfg;306779$48

That anchors the suffix for both the endswith() and splitext() cases... so if the previous behaviour was to match TARGETS as a suffix, it would have needed to be specified as ".TARGETS" in order to work.

It's possible there are such bugs, but all my test counts locally line up before and after. (And I'll double-check again, just to be sure.) I believe the only cases where the behaviour would actually differ is if the old pattern had a latent bug.


Repository:
  rL LLVM

https://reviews.llvm.org/D34855





More information about the llvm-commits mailing list