On Thu, Jan 9, 2014 at 3:44 PM, Alp Toker <[email protected]> wrote: > > On 09/01/2014 11:36, Chandler Carruth wrote: > > >> On Thu, Jan 9, 2014 at 3:15 AM, Alp Toker <[email protected] <mailto: >> [email protected]>> wrote: >> >> __builtin functions are introduced by the implementation (either >> the compiler or system header declarations) so there's no problem. >> >> Sanitizer is a system library so it's free to use the identifiers >> it wants. >> >> >> The sanitizers aren't system libraries as I define them (things in >> /usr/include or /usr/lib). They're compiler runtime libraries. They get >> installed in lib/clang/... along with other compiler resources. They are >> tightly coupled and version locked with the compiler itself as they >> interface with compiler generated instrumentation in most cases. The leak >> sanitizer is unusual in that it *could* be separated into something other >> than the compiler runtime libraries, but we explicitly chose not to as a >> design goal: we want to be able to offer it coupled with address sanitizer. >> >> Also, system libraries aren't free to use reserved names... so maybe you >> meant something different from what is commonly called a system library on >> linux/mac? >> > > s/libraries/headers/ clearly. > > > > >> >> TableGen, on the other hand, is not a part of the implementation. >> >> >> No one is claiming that it is. >> >> >> >> >> Can you clarify exactly what the error is and why? It's >> possible we could provide a macro of some kind in the >> sanitizer headers to bundle this functionality up if we need >> some special marking of this for checkers, but without more >> information its hard to hell what the actual problem is. >> >> >> These two commits _introduced_ names in the reserved namespace. >> >> >> Reserved names are reserved for any use. That can include an >> implementation specifically blessing the introduction of a name that uses >> double underscores. >> > > That's a pretty abstract argument and the standard says nothing like that. > > Regardless, the implementation I'm using here doesn't have such a > provision. > > > It may be unusual, but I don't think there is anything in the standard >> that makes this a non-conforming extension. >> > > C++ 7.1.3 Reserved identifiers > "If the program declares or defines an identifier in a context in which it > is reserved or defines a reserved identifier as a macro name, the behavior > is undefined." > > > >> >> The checker I have is a strict warning flag I've been trying to >> encode the rules from the standard and in this case the diagnostic >> looks legitimate. >> >> >> Ok, well, we disagree. ;] >> >> If the concern is that this is an ugly interface, I'm somewhat >> sympathetic and we could explore better ways to expose this interface to >> users, but I don't think this is incorrect as is, it just looks slightly >> sub-optimal. And I don't think there is any standards problem here, as this >> is and intentional extension provided by Clang+LSan. >> >> Now, one thing that I meant to ask for and didn't is to hid all of this >> behind some macro which is used to enable LSan. If we're not compiling with >> a compiler that supports this extension, it seems reasonable (if not likely >> of practical importance) to avoid touching the reserved name. >> > > > Kostya's commit is the only instance I can find of this violation in > dozens of large software projects. It's a first grader mistake (in the > interface, not necessarily the commit), not something that should exist in > a compiler toolchain codebase, thus my concern :-) > > If you really want to keep it however, let's compromise and you can put it > behind a macro. I do however want to give a strong nudge to get this > interface fixed. A single-underscore or no-underscore variant would do -- > nothing fancy needed.
But the choice of the prefix is deliberate -- we don't want to clash with any user code. > > > Alp. > > > -- > http://www.nuanti.com > the browser experts > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
