Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#276 closed change (rejected)

[meta] Stop using ATL

Reported by: fhd Assignee: eric@…
Priority: P5 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords: meta
Cc: trev, fhd, eric@… Blocked By: #1232, #1233, #1234, #1467
Blocking: #1163 Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5765952465534976/

Description (last modified by eric@…)

Background

We're currently using some things from ATL, which requires installing the Windows Driver Kit 7.1.0 (newer versions won't work) to build ABP for IE. If we got rid of this, building ABP would only require Visual Studio Express and Windows SDK.

What to change

Replace anything we use from ATL with either our own code, or a third party dependency that we can include in source.

Ticket Status Resolution Summary Component Owner
#1232 closed fixed [Stop using ATL] Replace ATL::CSimpleArray with std::set Adblock-Plus-for-Internet-Explorer eric@…
#1233 closed rejected [Stop using ATL] Replace ATL::CComBSTR Adblock-Plus-for-Internet-Explorer
#1234 new [Stop using ATL] Replace ATL::CString with std::wstring Adblock-Plus-for-Internet-Explorer eric@…
#1467 new [Stop using ATL] Convert ATL mutexes to standard library ones. Adblock-Plus-for-Internet-Explorer


Change History (25)

comment:1 Changed 5 years ago by trev

  • Cc trev added
  • Description modified (diff)

comment:2 Changed 5 years ago by eric@…

  • Owner set to eric@…
  • Status changed from new to assigned

comment:3 Changed 5 years ago by eric@…

I've started a Discourse thread on this issue.

comment:4 Changed 5 years ago by fhd

  • Cc fhd added

As Wladimir said there, let's move the discussion here.

comment:5 Changed 5 years ago by philll

  • Status changed from assigned to new

The assigned state will be dropped by #403

comment:6 Changed 5 years ago by eric@…

This guy has figured it out: Writing a BHO in Plain C++.

The advantage of the approach here is that it's a minimal COM object needed to act as a BHO. There's nothing extraneous here, very much unlike the plugin class definition that the ATL wizard outputs (which is what seems to be in our code base). I was rather worried that I'd have to reverse engineer what we had to keep and what could be discarded. This article act as a road map for a rewrite.

That said, there are also some ATL utility classes we use, but those were never going to be where the bulk of the work lay.

comment:7 Changed 5 years ago by eric@…

  • Cc eric@… added

comment:8 Changed 5 years ago by eric@…

There are five ATL headers used. At the coarsest level, the scope of this issue is to eliminate these headers.

atlbase.h
atlstr.h
atltypes.h
atlcom.h
atlhost.h

This is the exact list of all ATL symbols used.

_AtlBaseModule
CAtlBaseModule
CComAggObject
CComModule
CComAutoCriticalSection
CComBSTR
CComClassFactory
CComCoClass
CComCreator
CComCreator2
CComMultiThreadModel
CComObject
CComObjectNoLock
CComObjectRootEx
CComPtr
CComQIPtr
CComSingleThreadModel
CComVariant
CRect
CSimpleArray
CString
CStringA
CStringW
CW2A
IDispatchImpl
IObjectWithSiteImpl
OLE2T

By far the most-used symbols are CString and the other string classes.

comment:9 Changed 5 years ago by eric@…

  • Review URL(s) modified (diff)

comment:10 Changed 5 years ago by eric@…

  • Blocking 1163 added

comment:11 Changed 5 years ago by fhd

  • Platform set to Unknown

Is the current plan still to replace all of ATL? From what I've heard you decided to focus on low hanging fruits, like CString. Seems reasonable not too put too much effort into it.

comment:12 Changed 5 years ago by sergz

Is the current plan still to replace all of ATL?

My vote is to keep using ATL, it will definitely result in more robust application, less and clearer code, as well as save a lot of resources, especially in a long term.

ATL::CString and others

I would avoid emphasizing of it, just say, that the new code should use std::wstring or the proper structure, and if we touch the old code and it does not cause avalanche-like changes we can replace ATL::CString by std::string.
BTW, what is the main problem with CString except different from std interface? In some cases it's very convenient and replacing it by our hand-made functions violates NIH syndrome.

Last edited 5 years ago by sergz (previous) (diff)

comment:13 Changed 5 years ago by fhd

BTW, what is the main problem with CString except different from std interface? In some cases it's very convenient and replacing it by our hand-made functions violates NIH syndrome.

From what I see, these hand-made functions are mostly wrappers around functions that work with CString, that's good IMO, assuming we stick to std::string functionality as much as possible - duplicating things std::string can do would not be good.

In general, it seems like a good idea to use just one string class across the code base. It's hard for me to make a general statement though, I have seen very little of how we use CString. I don't think we have to be religious about it if it makes the code worse - assuming we are not going to get rid of ATL completely.

comment:14 Changed 5 years ago by eric@…

  • Using a single string class consistently across our code base is a clear good. (I'm counting wstring and string as the same for the purposes of this design discussion.) As of today we have five string classes:
    • wstring (std)
    • BSTR (Microsoft)
    • CComBSTR (Microsoft)
    • CString (Microsoft)
    • BString (ours)
    We're using all the Microsoft classes semantically at some point in the code, though by far the bulk of such uses are in CString. One major goal for string types is to get all the semantic uses into wstring.
  • Most occurrences of CString, however, either do not involve string semantics or do so only in very light ways (e.g. empty(), length(), and concatenation). A large number are just used to pass values around unaltered and unexamined.
  • CString is anything but low-hanging fruit. It's textually extensive. It's also worth doing in order to simplify the code. Converting back and forth between morally-equivalent types is nothing but junk code; you might really want it, but that doesn't mean it's good for you.
  • The functions I wrote were to replace member functions in CString. Some did not have direct equivalents in the standard library (such those to trim whitespace). Some were syntactic sugar around existing library functions (such as begins_with as a particular form of compare). The case of CString::tokenize was a bit special; it was expedient to replicate the semantics of the CString function. Overall, it's rather a small amount of code we needed that wasn't already in wstring.
Last edited 5 years ago by eric@… (previous) (diff)

comment:15 Changed 5 years ago by eric@…

  • Blocked By 1232 added

comment:16 Changed 5 years ago by eric@…

  • Description modified (diff)

comment:17 Changed 5 years ago by eric@…

  • Blocked By 1233 added

comment:18 Changed 5 years ago by eric@…

  • Blocked By 1234 added

comment:19 Changed 5 years ago by fhd

  • Keywords meta added
  • Platform changed from Unknown to Internet Explorer

comment:20 Changed 5 years ago by fhd

  • Summary changed from Stop using ATL to [meta] Stop using ATL

comment:21 Changed 5 years ago by fhd

  • Review URL(s) modified (diff)

comment:22 Changed 5 years ago by eric@…

  • Blocked By 1467 added

comment:23 Changed 5 years ago by oleksandr

  • Priority changed from P4 to P5

comment:24 Changed 5 years ago by oleksandr

  • Resolution set to rejected
  • Status changed from new to closed

comment:25 Changed 5 years ago by trev

For reference, replacing ATL completely is no longer planned - the benefits aren't worth the effort.

Note: See TracTickets for help on using tickets.