[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D35083#804623, @jingham wrote:

> As a general practice requiring a wrapper like this for every use of a should be locked object would make the code noisy and hard to read.  The only error you would be protecting against is that somebody used the entity after the lock went out of scope.  You could use some kind of markup to enforce this requirement, but you also have to be pretty sloppy to make this kind of error, so I'm not sure this it worth going to great lengths to protect against.
>
> In this limited use, I guess I don't object seriously, but don't like this as a general pattern.


+1. For limited use, I am fine with leaving this up to the owners discretion, but I would object to a more widespread usage.



================
Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:24
+template <typename FnType>
+static decltype(std::declval<FnType>()(std::declval<ASTSourceMap&>()))
+WithExclusiveSourceMap(FnType fn) {
----------------
spyffe wrote:
> 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)>'
> ```
that should probably be something like std::result_of<FnType(ASTSourceMap&)>


================
Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:55
+  });
   g_TotalSizeOfMetadata += m_metadata.size();
 }
----------------
Since you're worrying about this being run concurrently, it looks like you should also protect the access to this global variable (possibly by making it atomic).


Repository:
  rL LLVM

https://reviews.llvm.org/D35083