[PATCH] D78202: [FileCheck] - Refactor the code related to string arrays. NFCI.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:539
+  // is probably a bit subtle. So we copy strings instead.
+  BumpPtrAllocator StrAlloc;
+  for (StringRef Prefix : CheckPrefixes)
----------------
grimar wrote:
> MaskRay wrote:
> > `--check-prefix`, `--implicit-check-not` and `-D` are by no means performance/memory usage critical... Using a `BumpPtrAllocator` does not seem to improve the code ?
> > 
> > Can other cleanups in this patch be retained if you don't change the 3 variables to `std::vector<StringRef>`?
> I think having
> 
> ```
>   std::vector<StringRef> CheckPrefixes;
>   std::vector<StringRef> ImplicitCheckNot;
>   std::vector<StringRef> GlobalDefines;
> ```
> 
> instead of  
> 
> ```
>   std::vector<std::string> CheckPrefixes;
>   std::vector<std::string> ImplicitCheckNot;
>   std::vector<std::string> GlobalDefines;
> ```
> 
> Improves the code by itself as readers should not think about why "std::string" was used.
> We can revert to the previous revision of the patch as it works fine even without
> adding a "BumpPtrAllocator".
Also, for example this patch changes:

```
Error FileCheckPatternContext::defineCmdlineVariables(
    std::vector<std::string> &CmdlineDefines, SourceMgr &SM) {
```

to

```
Error FileCheckPatternContext::defineCmdlineVariables(ArrayRef<StringRef> CmdlineDefines,
                               SourceMgr &SM);
```

Using of `std::string` would require changing `CmdlineDefines` to be `ArrayRef<std::string>`,
i.e. `std::string` is spreading for no reason and the idea of this patch is to stop it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78202/new/

https://reviews.llvm.org/D78202