[PATCH] D111709: [Flang] flang-omp-report replace std::vector's with llvm::SmallVector

[David Truby via Phabricator via llvm-commits]

DavidTruby added inline comments.


================
Comment at: flang/examples/flang-omp-report-plugin/flang-omp-report-visitor.h:101
+  llvm::SmallVector<OmpWrapperType *> ompWrapperStack;
+  std::map<OmpWrapperType *, llvm::SmallVector<ClauseInfo>> clauseStrings;
   Parsing *parsing{nullptr};
----------------
clementval wrote:
> DavidTruby wrote:
> > clementval wrote:
> > > DavidTruby wrote:
> > > > clementval wrote:
> > > > > DavidTruby wrote:
> > > > > > just putting this here as a note (this should be done in a separate patch):
> > > > > > This `std::map` should probably be changed to an `llvm::DenseMap` or similar
> > > > > You can use `std::unordered_map` here instead. 
> > > > Use of `std::unordered_map` is highly discouraged in llvm because it has portability issues across implementations. See https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options for more details
> > > Unordered_set is discourage but not unordered_map. It is used widely in the code base
> > I'm not sure why `unordered_set` would be discouraged but not `unordered_map`, they're implemented the same way so would surely exhibit the same issues? The section of the manual I linked to specifically says to avoid that "hash map like" containers and then (I think erroneously) calls out `unordered_set` when I presume it means `unordered_map`. Perhaps that needs a fix in the manual?
> > 
> > Regardless with a quick grep through llvm I don't see many uses of `unordered_map` outside of libcxx (which of course defines it), especially compared to uses of DenseMap.
> You probably read between the line because the section you linked does not says avoid hash map like container anywhere. It just mentions that the STL provides such containers.
> 
> > The STL provides several other options, such as std::multimap and the various “hash_map” like containers (whether from C++ TR1 or from the SGI library). We never use hash_set and unordered_set because they are generally very expensive (each insertion requires a malloc) and very non-portable.
> 
> 
I'm actually fairly sure this is a mistake in the guide, because this section is talking exclusively about maps and then starts talking about sets (which are covered in a separate section) for no apparent reason! That ambiguity should probably be taken up in a separate review. 


Regardless elsewhere we have the following :
> When both C++ and the LLVM support libraries provide similar functionality, and there isn’t a specific reason to favor the C++ implementation, it is generally preferable to use the LLVM library. For example, llvm::DenseMap should almost always be used instead of std::map or std::unordered_map, and llvm::SmallVector should usually be used instead of std::vector.

Which seems to pretty clearly discourage either map or unordered_map in favour of DenseMap




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111709