[CXX-757] document{} << const char* casts argument to bool Created: 03/Dec/15 Updated: 11/Jan/16 Resolved: 08/Dec/15 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | BSON |
| Affects Version/s: | 0.2.0 |
| Fix Version/s: | 3.0.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Scott Deerwester | Assignee: | Mira Carey |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
| Epic Link: | C++11 Driver MongoDB 3.2 |
| Description |
|
When constructing a query in the C++ driver, the following: const char *term = "value"; casts the second argument (term) to bool, creating a query "attribute"=true instead of the expected "attribute" = "value". The example assumes that the attached JSON file has been loaded to collection testCollection in database test. |
| Comments |
| Comment by Githook User [ 08/Dec/15 ] | ||
|
Author: {u'username': u'hanumantmk', u'name': u'Jason Carey', u'email': u'jcarey@argv.me'}Message: Disables append for non-char pointer types
removes appends for string literals && enables append for char pointers
| ||
| Comment by Mira Carey [ 04/Dec/15 ] | ||
|
After some conversation on our side, it seems like most of my team is leaning towards the: "all else being equal, support const char*". I've also had a chance to play around with disabling non-char pointers, which seems to work. Code review for details: | ||
| Comment by Scott Deerwester [ 04/Dec/15 ] | ||
|
Thanks, Jason. I really appreciate your obvious depth in considering all of the factors behind a potential change. It's too easy to be reactive, rather than responsive, and it's refreshing to see somebody getting it right. | ||
| Comment by Mira Carey [ 04/Dec/15 ] | ||
|
Scott, Considering your reply:
I need to do a little more fiddling, but I think I've figured out a rough list of priorities:
| ||
| Comment by Scott Deerwester [ 04/Dec/15 ] | ||
|
Thanks for the reply, Jason. I would lean fairly strongly toward your second option, for two primary reasons:
In short, the cost-benefit analysis of breakage vs ms while constructing a query weighs heavily in favor of eliminating the breakage. | ||
| Comment by Mira Carey [ 03/Dec/15 ] | ||
|
Scott, Unfortunately, this basically boils down the age old problem in C++ that bool's and pointers are a bit too close together (you're seeing the built-in pointer conversion to bool, over the provided implicit string_view constructor). And while there is the obvious solution (a const char* overload), we're actually trying to pick up a string literal template match (which saves us a memchr and a copy (because we have extra lifetime info)) of the form:
Which is technically a worse match than const char* (because array to pointer decay is closer than a template) So there are a couple of ways forward: 1. Status QuoNot the best, because users can accidentally pass pointer types and get them bool converted, which can lead to silent data corruption issues. 2. Addition of const char* overloadBetter, because we do what the user meant. But worse, because we traded runtime performance. Also, users can still accidentally call the boolean overload with other char types (wide chars on windows lets say). 3. Removal of the boolean overloadMakes a lot more things not compile
But the perf is exactly where we wanted it and no one sees anything unexpected at runtime I'm leaning pretty heavily towards 3, despite the breakage, because it offers perf and safety for a little usability. What are your thoughts? |