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

[Brian Gesiak via Phabricator via llvm-commits]

modocache accepted this revision.
modocache added inline comments.
This revision is now accepted and ready to land.


================
Comment at: utils/lit/lit/util.py:139
+            filename in exclude_filenames or
+            not any(filename.endswith(sfx) for sfx in suffixes)):
+            continue
----------------
dlj wrote:
> 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.
Ah OK. Great idea to make sure the test counts match. That sounds fine to me -- thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D34855