spyffe added a comment.
Responded to Lang's comments inline.
**Jim**: you say this patch "doesn't actually enforce that you take the lock to
get the SourceMap." How do you get the source map otherwise? It's static
inside the function so nothing can see it outside. Do you mean that it's still
possible to have the lambda make an alias to the reference and return?
**Pavel**: if I have `GetSourceMap()` take a `unique_lock&` and leave the
return value as it is, then clients get an unguarded alias to `s_source_map` to
play with regardless of whether they're holding the lock or not. We have to
trust that they hold on to the lock as long as they need to – or use
`GUARDED_BY`, adding more complexity for something that
`WithExclusiveSourceMap()` enforced by default. (Creating an escaping alias is
still an issue.)
I'm going to hold off for Lang's opinion on this. I feel like we have a few
workable solutions, and since this isn't API for anything even outside the
`.cpp` file picking one and going with it is fine.
================
Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:24
+template <typename FnType>
+static decltype(std::declval<FnType>()(std::declval<ASTSourceMap&>()))
+WithExclusiveSourceMap(FnType fn) {
----------------
lhames wrote:
> Does std::result_of<FnType>::type work as the return type?
>
> http://en.cppreference.com/w/cpp/types/result_of
No, it doesn't. If I change the declaration to
```
template <typename FnType>
static typename std::result_of<FnType>::type
WithExclusiveSourceMap(FnType fn) {
```
then the compiler can no longer resolve calls:
```
…/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:25:1: Candidate template
ignored: substitution failure [with FnType = (lambda at
…/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:39:7)]: implicit
instantiation of undefined template 'std::__1::result_of<(lambda at
…/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:39:7)>'
```
================
Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:28
static ASTSourceMap *s_source_map = new ASTSourceMap;
- return *s_source_map;
+ static std::mutex s_source_map_mutex;
+
----------------
lhames wrote:
> Should this be on a context object of some kind (ASTContext?).
No, it shouldn't. This is intended to be global.
Repository:
rL LLVM
https://reviews.llvm.org/D35083
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
